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

Prototype for the advice api #4341

Closed
wants to merge 1 commit into from

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Jul 19, 2023

This demonstrates that an option on histogram instruments for providing explicit bucket boundaries can be added in a backwards-compatible manner.

This prototype is missing:

  • correct naming ("advice" should be here somewhere, but it seemed clunky), so I skipped it.
  • this is not a field on "Instrument", as required by the specification. That would seem to require our Instrument struct to export an Advice field. But our Instrument struct is used by Views, and it seems incorrect to select on advice from a view.
  • I didn't make the bucket boundaries identifying, as required by the spec. It would be easy to do that in this prototype by adding the buckets to instID. See Ensure the advice API can be added to the metric SDK #4162 (comment)
  • Tests beyond a single unit test

Prototype for #4162

@dashpole dashpole force-pushed the advice_api_prototype branch from 45e03f1 to c65b194 Compare July 19, 2023 18:08
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #4341 (9915c30) into main (e08359f) will decrease coverage by 0.1%.
The diff coverage is 77.5%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4341     +/-   ##
=======================================
- Coverage   83.4%   83.3%   -0.1%     
=======================================
  Files        185     185             
  Lines      14332   14354     +22     
=======================================
+ Hits       11958   11971     +13     
- Misses      2147    2156      +9     
  Partials     227     227             
Impacted Files Coverage Δ
metric/instrument.go 95.1% <0.0%> (-4.9%) ⬇️
metric/syncfloat64.go 93.7% <0.0%> (-6.3%) ⬇️
metric/syncint64.go 93.7% <0.0%> (-6.3%) ⬇️
sdk/metric/instrument.go 92.1% <100.0%> (+0.3%) ⬆️
sdk/metric/meter.go 85.9% <100.0%> (+<0.1%) ⬆️
sdk/metric/pipeline.go 91.3% <100.0%> (+0.1%) ⬆️

... and 1 file with indirect coverage changes

@dashpole dashpole force-pushed the advice_api_prototype branch from c65b194 to 9915c30 Compare July 19, 2023 18:11
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Jul 19, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Jul 19, 2023

LGTM. I've added this to the SIG meeting agenda for tomorrow so we can review and close the issue.

@pellared
Copy link
Member

This demonstrates that an option on histogram instruments for providing explicit bucket boundaries can be added in a backwards-compatible manner.

LGTM

@MrAlias
Copy link
Contributor

MrAlias commented Jul 20, 2023

Reviewed in the SIG, and we concluded this satisfies the PoC requirement. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants