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

Remove Context from sync instruments #1076

Merged
merged 11 commits into from
Jun 15, 2023

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented May 24, 2023

Opening a PR which removes Context as part of the parameters to the measurement reporting API for Counter. If this is agreed, I can remove it for other sync instruments as well. (Updated to remove for all sync instruments) I am not sure if this was intentionally kept, as the Metrics API spec removed this long ago(?) or it never existed in the spec. The SDK could infer the current implicit context OR we could introduce a new ctr.AddWithContext() accepting explicit context if there is a need for passing non-implicit context..

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@cijothomas cijothomas requested a review from a team as a code owner May 24, 2023 04:37
@hdost
Copy link
Contributor

hdost commented May 24, 2023

I believe @jtescher added it last year.

@jtescher
Copy link
Member

I think the main use here would be for exemplars, but it's not being used yet. Could work that through first and see if we can delete it after that, or I suppose alternatively we could remove it now and re-add it if needed for exemplars.

@lalitb
Copy link
Member

lalitb commented May 24, 2023

It seems the specs lets the language implementation decide whether to pass the Context parameter explicitly, or else use the current active Context. C++ and Java provides overload methods with Context parameter, Rust has this parameter as mandatory, while other languages use the currently active Context.

@cijothomas
Copy link
Member Author

I think the main use here would be for exemplars, but it's not being used yet. Could work that through first and see if we can delete it after that, or I suppose alternatively we could remove it now and re-add it if needed for exemplars.

Would you agree to the following plan:

  1. Remove the Context parameter from instruments now. (This PR)
  2. When working on Exemplar + the capability to add attributes from Baggage (the only 2 features in Metrics that are spec-ed out which require Context), we can start with relying on the implicit context without requiring user to explicitly pass it.
  3. If there is a desire to provide a different context than the implicit one, introduce a new api (counter.AddWithContext()) that accepts the context (like C++, Java)

@lzchen
Copy link
Contributor

lzchen commented May 25, 2023

Title should be renamed to only address sync counters

@jtescher
Copy link
Member

Would you agree to the following plan:

  1. Remove the Context parameter from instruments now. (This PR)
  2. When working on Exemplar + the capability to add attributes from Baggage (the only 2 features in Metrics that are spec-ed out which require Context), we can start with relying on the implicit context without requiring user to explicitly pass it.
  3. If there is a desire to provide a different context than the implicit one, introduce a new api (counter.AddWithContext()) that accepts the context (like C++, Java)

Yeah I think that's fine, we have both options when creating traces, I think long term having both is reasonable.

@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Patch coverage: 69.2% and project coverage change: -0.1 ⚠️

Comparison is base (e773f92) 50.7% compared to head (0647224) 50.7%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1076     +/-   ##
=======================================
- Coverage   50.7%   50.7%   -0.1%     
=======================================
  Files        165     165             
  Lines      19808   19808             
=======================================
- Hits       10059   10058      -1     
- Misses      9749    9750      +1     
Impacted Files Coverage Δ
opentelemetry-api/src/metrics/noop.rs 5.5% <0.0%> (ø)
opentelemetry-prometheus/src/lib.rs 84.5% <ø> (ø)
opentelemetry-sdk/src/lib.rs 100.0% <ø> (ø)
...etry-sdk/src/testing/metrics/in_memory_exporter.rs 0.0% <ø> (ø)
opentelemetry/src/lib.rs 100.0% <ø> (ø)
stress/src/metrics.rs 14.2% <0.0%> (ø)
...entelemetry-api/src/metrics/instruments/counter.rs 30.3% <100.0%> (ø)
...telemetry-api/src/metrics/instruments/histogram.rs 64.5% <100.0%> (ø)
...try-api/src/metrics/instruments/up_down_counter.rs 26.6% <100.0%> (ø)
opentelemetry-sdk/src/metrics/instrument.rs 30.1% <100.0%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cijothomas
Copy link
Member Author

@jtescher @hdost @lzchen This is rebased from main, and is ready for merge, once approved! Please take a look! @TommyCpp I'd like to ensure this is part of the upcoming release (as metrics already has breaking change, so why not have them all in one go!)

@jtescher jtescher merged commit a08c640 into open-telemetry:main Jun 15, 2023
11 checks passed
@cijothomas cijothomas deleted the cijothomas/remove-ctr-metric1 branch June 16, 2023 19:24
ttys3 added a commit to ttys3/axum-otel-metrics that referenced this pull request Jul 17, 2023
ttys3 added a commit to ttys3/axum-otel-metrics that referenced this pull request Jul 17, 2023
BREAKING CHANGE: otel api has context removed

Refs: open-telemetry/opentelemetry-rust#1076
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

Successfully merging this pull request may close these issues.

None yet

5 participants