Opentelemetry-go: Suggestion: rename `CurrentSpan`/`SetCurrentSpan`

Created on 10 Oct 2019  路  10Comments  路  Source: open-telemetry/opentelemetry-go

Thoughts on renaming these to SpanFromContext and ContextWithSpan to be more idiomatic?

All 10 comments

Hey, @iredelmeier I would like to work on this. Could you help me what are exact changes required?
Could you please point me some good first issues in getting started to contribute to open-telemetry.

@VineethReddy02 the change for this should basically be a find-and-replace for the function provided in current.go (which should probably also be renamed!) and their usages.

With the caveat that a PR implementing this suggestion may get rejected on the basis of the suggestion itself getting rejected, this would definitely be a good first issue! Inconveniently, I'm unable to modify issue labels in this repo.

Some more good first issues (definitely non-exhaustive!):

  • #181
  • #184 (blocked on some decision-making)
  • #186 (possibly)
  • #192
  • #195

@VineethReddy02 are you still able to work on this?

Hey @iredelmeier so what CurrentSpan & SetCurrentSpan should be renamed to?

@VineethReddy02 CurrentSpan -> SpanFromContext and SetCurrentSpan -> ContextWithSpan

@rakyll proposed "WithSpan" as opposed to "ContextWithSpan". What do you think? I do feel that "Context" adds stutter here. If we went with "WithSpan", what's the opposite name? (I still like CurrentSpan). Also if we went with "WithSpan", there's a Tracer.WithSpan, similar to trace.WithRegion in the builtin library, that creates confusion.

I like more trace.ContextWithSpan (it is a little verbose but more clear than trace.WithSpan that could be misleading) and trace.CurrentSpan.

Although the PR merged, I am open to renaming trace.SpanFromContext to trace.CurrentSpan.

stale issue.

Was this page helpful?
0 / 5 - 0 ratings