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

Add embedded package to trace API #4620

Merged
merged 11 commits into from Oct 19, 2023
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Oct 12, 2023

Resolve #4545
Resolve #4160

Similar to the metric API, add an embedded package for the trace API that includes types embedded in all API interfaces. This ensures implementers of the API will have to make a choice about their implementation's default behavior.

To ensure implementors have a noop options available to them for the default behavior, this also adds a trace/noop package with exported concrete No-Op types they can embed.

@MrAlias MrAlias force-pushed the trace-embedded branch 2 times, most recently from 33f2dd5 to b7b7d2d Compare October 13, 2023 14:31
@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Merging #4620 (725d34f) into main (da343ab) will increase coverage by 0.0%.
The diff coverage is 94.1%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4620   +/-   ##
=====================================
  Coverage   81.3%   81.3%           
=====================================
  Files        222     223    +1     
  Lines      17690   17721   +31     
=====================================
+ Hits       14393   14423   +30     
- Misses      2997    2998    +1     
  Partials     300     300           
Files Coverage Δ
bridge/opentracing/bridge.go 63.7% <ø> (ø)
bridge/opentracing/provider.go 100.0% <ø> (ø)
bridge/opentracing/wrapper.go 100.0% <ø> (ø)
internal/global/trace.go 89.0% <ø> (ø)
sdk/trace/span.go 87.9% <ø> (ø)
sdk/trace/tracer.go 100.0% <ø> (ø)
trace/noop.go 61.9% <ø> (-4.8%) ⬇️
trace/noop/noop.go 100.0% <100.0%> (ø)
trace/trace.go 98.2% <ø> (ø)
bridge/opentracing/internal/mock.go 67.0% <0.0%> (ø)
... and 1 more

@MrAlias MrAlias force-pushed the trace-embedded branch 7 times, most recently from ff4547c to f0c3c15 Compare October 13, 2023 18:19
@MrAlias MrAlias added pkg:API Related to an API package area:trace Part of OpenTelemetry tracing labels Oct 13, 2023
@MrAlias MrAlias added this to the v1.20.0 milestone Oct 13, 2023
@MrAlias MrAlias marked this pull request as ready for review October 13, 2023 18:34
trace/doc.go Outdated Show resolved Hide resolved
trace/noop.go Show resolved Hide resolved
Co-authored-by: David Ashpole <dashpole@google.com>
@pellared
Copy link
Member

I think that this also solves #3277 as well as #4160

If you agree then I suggest updating the PR description.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 17, 2023

I think that this also solves #3277 as well as #4160

If you agree then I suggest updating the PR description.

I think #4160 will be resolved by this. I can update the description. 👍

I don't know about #3277. That is a bug with the project's versioning policy. This doesn't change the versioning policy so I don't see it resolves that.

I also think #3277 should be closed, as the history shows. It is something we don't plan to change given the general OTel versioning policy.

@MrAlias MrAlias merged commit 1e1cc90 into open-telemetry:main Oct 19, 2023
25 checks passed
@MrAlias MrAlias deleted the trace-embedded branch October 19, 2023 17:16
@katiehockman
Copy link

@MrAlias do you have a planned date for when this change will be released? We have a PR ready that will make the necessary changes in our API implementation, but we'd like to be able to plan ahead to merge and release the changes as soon after the release as possible, to lessen the time our users could be impacted.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 24, 2023

@MrAlias do you have a planned date for when this change will be released? We have a PR ready that will make the necessary changes in our API implementation, but we'd like to be able to plan ahead to merge and release the changes as soon after the release as possible, to lessen the time our users could be impacted.

@katiehockman we are waiting on two PRs to be merged before the next milestone is reached. I'm not really sure how long that will take given I would have guessed they would already have been merged at this point. I plan to discuss the two PRs there at the next SIG meeting, so hopefully we can get that release out in a week or two.

Should I ping you in the release prep PR for the next release? Or is there a better way to give you a heads up?

@katiehockman
Copy link

Should I ping you in the release prep PR for the next release?

Yep that works! Or just ping me here when you have a more finalized planned date of release. Thank you

@EdSchouten
Copy link

Why did #4545 get locked? Two people comment that this causes issues for them. Then without any response whatsoever it gets closed. That seem to be very inclusive to me.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 14, 2023

Why did #4545 get locked? Two people comment that this causes issues for them. Then without any response whatsoever it gets closed. That seem to be very inclusive to me.

@EdSchouten if you have an issue to report please open a new issue. Do not comment on merged PRs. Comments such as these are treated as spam and will result in the discussion being locked.

@open-telemetry open-telemetry locked as resolved and limited conversation to collaborators Nov 14, 2023
@pellared
Copy link
Member

@EdSchouten I responded in #4714. Please feel free to propose a better solution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:trace Part of OpenTelemetry tracing pkg:API Related to an API package
Projects
None yet
6 participants