Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: rename CurrentSpan/SetCurrentSpan #185

Closed
iredelmeier opened this issue Oct 10, 2019 · 10 comments
Closed

Suggestion: rename CurrentSpan/SetCurrentSpan #185

iredelmeier opened this issue Oct 10, 2019 · 10 comments
Assignees
Milestone

Comments

@iredelmeier
Copy link
Member

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

@VineethReddy02
Copy link
Contributor

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.

@iredelmeier
Copy link
Member Author

@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!):

@rakyll
Copy link
Contributor

rakyll commented Nov 11, 2019

See also #299.

@iredelmeier
Copy link
Member Author

@VineethReddy02 are you still able to work on this?

@VineethReddy02
Copy link
Contributor

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

@iredelmeier
Copy link
Member Author

@VineethReddy02 CurrentSpan -> SpanFromContext and SetCurrentSpan -> ContextWithSpan

@jmacd
Copy link
Contributor

jmacd commented Dec 11, 2019

@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.

@paivagustavo
Copy link
Member

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

@jmacd
Copy link
Contributor

jmacd commented Dec 11, 2019

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

@rghetia
Copy link
Contributor

rghetia commented Mar 26, 2020

stale issue.

@rghetia rghetia closed this as completed Mar 26, 2020
hstan referenced this issue in hstan/opentelemetry-go Oct 15, 2020
* Upgrade to v0.10.0 version

* Upgrade to v0.10.0 code changes

* Update Changelog
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants