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

feat(sdk-metrics): added synchronous gauge to SDK #4565

Merged

Conversation

clintonb
Copy link
Contributor

Which problem is this PR solving?

Split from #4528, relates to #4296

Short description of the changes

Added SDK for synchronous gauge.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@clintonb clintonb requested a review from a team March 21, 2024 04:45
@clintonb
Copy link
Contributor Author

FYI @pichlermarc

@clintonb
Copy link
Contributor Author

@pichlermarc given the SDK depends on the API, is the order wrong here? It seems we should update the API first.

@pichlermarc
Copy link
Member

@clintonb ah, this is the counter-intuitive situation I mentioned over at #4565. 🙂

Since @opentelemetry/sdk-metrics has a peer-dependency on @opentelemetry/api and that peer dependency allows for using API versions starting at 1.3.0, we can only use 1.3.0 types. Since Gauge will only be added in 1.9.0 we can't use it in this package (the current build failure simulates what would happen to a user when they have an older API - for instance 1.8.0 installed that does not have the Gauge type available).

The workaround is to copy the Gauge type from the API package to the @opentelemetry/sdk-metrics package. Since it's then copied 1:1 it'll be type-compatible with the API's version.

We're currently discussing ways to improve this situation with SDK 2.0 (many edge cases, difficult topic), but in the meantime this is the way forward to add the feature. 🙂

@clintonb clintonb force-pushed the clintonb/synchronous-gauge-sdk branch from 6131e5e to 95f7f66 Compare March 23, 2024 21:26
@pichlermarc pichlermarc changed the title feat(instrumentation): added synchronous gauge to SDK feat(sdk-metrics): added synchronous gauge to SDK Mar 25, 2024
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment, then I think this should be good to merge. 🙂
Thanks for working on this @clintonb 🚀

packages/sdk-metrics/src/Meter.ts Show resolved Hide resolved
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Merging #4565 (512a359) into main (d66e1d7) will decrease coverage by 1.57%.
The diff coverage is 50.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4565      +/-   ##
==========================================
- Coverage   92.90%   91.33%   -1.57%     
==========================================
  Files         326      254      -72     
  Lines        9454     7112    -2342     
  Branches     2024     1614     -410     
==========================================
- Hits         8783     6496    -2287     
+ Misses        671      616      -55     
Files Coverage Δ
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 92.75% <50.00%> (-1.37%) ⬇️

... and 83 files with indirect coverage changes

@clintonb clintonb force-pushed the clintonb/synchronous-gauge-sdk branch from 1bbd61a to 96aefb2 Compare March 26, 2024 17:21
@clintonb
Copy link
Contributor Author

@pichlermarc I've rebased on main, added the @experimental comment flag, and updated the changelog.

Please take another look, and thanks for shepherding this!

@clintonb clintonb force-pushed the clintonb/synchronous-gauge-sdk branch from 96aefb2 to 1dbecf2 Compare March 31, 2024 16:41
@clintonb
Copy link
Contributor Author

@pichlermarc this circular dependency continues to hamper my development. The sdk-metrics tests depend on api, so I cannot increase test coverage to meet the threshold for merging.

@clintonb
Copy link
Contributor Author

See #4528 for complete test coverage.

@pichlermarc
Copy link
Member

The sdk-metrics tests depend on api, so I cannot increase test coverage to meet the threshold for merging.

I see. I'll merge this without the needed coverage and then we follow up on it in the next PR. 👍

@clintonb clintonb force-pushed the clintonb/synchronous-gauge-sdk branch from bbff221 to 512a359 Compare April 2, 2024 14:37
@clintonb
Copy link
Contributor Author

clintonb commented Apr 2, 2024

@pichlermarc rebased both branches/PRs. Ready for tests and merging.

@pichlermarc pichlermarc merged commit 928796d into open-telemetry:main Apr 2, 2024
18 of 20 checks passed
@clintonb clintonb deleted the clintonb/synchronous-gauge-sdk branch April 2, 2024 15:33
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
* feat(instrumentation): added synchronous gauge to SDK

* fixup! feat(instrumentation): added synchronous gauge to SDK

* fixup! feat(instrumentation): added synchronous gauge to SDK

* fixup! feat(instrumentation): added synchronous gauge to SDK

* fixup! feat(instrumentation): added synchronous gauge to SDK

* fixup! feat(instrumentation): added synchronous gauge to SDK
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.

2 participants