Skip to content

sdk/metric: Add experimental Enabled method to synchronous instruments - #6016

Merged
dmathieu merged 9 commits into
open-telemetry:mainfrom
codeboten:codeboten/enable-instrument
Dec 6, 2024
Merged

sdk/metric: Add experimental Enabled method to synchronous instruments#6016
dmathieu merged 9 commits into
open-telemetry:mainfrom
codeboten:codeboten/enable-instrument

Conversation

@codeboten

@codeboten codeboten commented Dec 4, 2024 •
edited by pellared
Loading

Copy link
Copy Markdown
Member

Fixes #6002

@codeboten
add experimental Enabled interface
2d0d4a5 
Opening this to get feedback on whether or not I'm headed in the right direction
based on the details listed in open-telemetry#6002. This
is a replacement for open-telemetry#5768

Fixes open-telemetry#6002

There are still some tests I would add, but overall I'm mostly looking to reviewers to let me know
if this is the correct approach (defining an interface in `x` and using that interface in the SDK)

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codecov

codecov Bot commented Dec 4, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.2%. Comparing base (3bb224b) to head (42b219a).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6016   +/-   ##
=====================================
  Coverage   82.1%   82.2%           
=====================================
  Files        273     273           
  Lines      23643   23647    +4     
=====================================
+ Hits       19433   19448   +15     
+ Misses      3865    3852   -13     
- Partials     345     347    +2

see 4 files with indirect coverage changes

pellared
pellared reviewed Dec 4, 2024
View reviewed changes

@pellared pellared left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work 👍 Overall, I think this is the direction we agreed on during the SIG meetings.

I think it would be good to also add an example in sdk/metric/example_test.go and add some benchmarks.

You can look at #5692 for reference.

Comment thread sdk/metric/internal/x/x.go Outdated
Comment thread CHANGELOG.md Outdated
Comment thread sdk/metric/instrument.go Outdated
Comment thread sdk/metric/internal/x/README.md Outdated
Comment thread sdk/metric/internal/x/x.go Outdated
Comment thread sdk/metric/internal/x/x.go Outdated
Comment thread sdk/metric/meter_test.go Outdated
Comment thread sdk/metric/meter_test.go Outdated
Comment thread sdk/metric/instrument.go Outdated
@pellared pellared changed the title add experimental Enabled interface sdk/metric: Add experimental Enabled method to synchronous instruments Dec 4, 2024
codeboten and others added 5 commits December 4, 2024 08:12
@codeboten @pellared
Apply suggestions from code review
4280aa4 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@codeboten
update test to use require, add context param, validate interface
7661cd6 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten
update readme
b4eb435 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten @pellared
Update CHANGELOG.md
28eabb3 
Co-authored-by: Robert Pająk <pellared@hotmail.com>
@codeboten
Merge branch 'main' into codeboten/enable-instrument
55be934
pellared
pellared reviewed Dec 5, 2024
View reviewed changes

@pellared pellared left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just one refactoring request from my side. The rest LGTM👍

Comment thread sdk/metric/meter_test.go Outdated
@pellared

pellared commented Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

I think it would be good to also add an example in sdk/metric/example_test.go and add some benchmarks.

After a second thought, I do not think it is really necessary.

There are still some tests I would add, but overall I'm mostly looking to reviewers to let me know if this is the correct approach

Is the PR description up to date? Looks like a pretty complete PR unless I am missing something 😉

dashpole
dashpole reviewed Dec 5, 2024
View reviewed changes
Comment thread sdk/metric/instrument.go
@codeboten
Merge branch 'main' into codeboten/enable-instrument
9f9bf02
@codeboten
apply feedback refactor
554ab27 
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
@codeboten

codeboten commented Dec 5, 2024

Copy link
Copy Markdown
Member Author

Updated the description, thanks for all the reviews/suggestions!

@pellared pellared mentioned this pull request Dec 5, 2024
Closed
@pellared pellared added this to the v1.33.0 milestone Dec 5, 2024
@dmathieu
dmathieu merged commit 0598dae into open-telemetry:main Dec 6, 2024
@MrAlias MrAlias mentioned this pull request Dec 12, 2024
Merged
MrAlias added a commit that referenced this pull request Dec 12, 2024
@MrAlias
Release v1.33.0 (#6035)
8c38f80 
### Added

- Add `Reset` method to `SpanRecorder` in
`go.opentelemetry.io/otel/sdk/trace/tracetest`. (#5994)
- Add `EnabledInstrument` interface in
`go.opentelemetry.io/otel/sdk/metric/internal/x`. This is an
experimental interface that is implemented by synchronous instruments
provided by `go.opentelemetry.io/otel/sdk/metric`. Users can use it to
avoid performing computationally expensive operations when recording
measurements. It does not fall within the scope of the OpenTelemetry Go
versioning and stability [policy](./VERSIONING.md) and it may be changed
in backwards incompatible ways or removed in feature releases. (#6016)

### Changed

- The default global API now supports full auto-instrumentation from the
`go.opentelemetry.io/auto` package. See that package for more
information. (#5920)
- Propagate non-retryable error messages to client in
`go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#5929)
- Propagate non-retryable error messages to client in
`go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`.
(#5929)
- Propagate non-retryable error messages to client in
`go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`.
(#5929)
- Performance improvements for attribute value `AsStringSlice`,
`AsFloat64Slice`, `AsInt64Slice`, `AsBoolSlice`. (#6011)
- Change `EnabledParameters` to have a `Severity` field instead of a
getter and setter in `go.opentelemetry.io/otel/log`. (#6009)

### Fixed

- Fix inconsistent request body closing in
`go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp`. (#5954)
- Fix inconsistent request body closing in
`go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`.
(#5954)
- Fix inconsistent request body closing in
`go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`.
(#5954)
- Fix invalid exemplar keys in
`go.opentelemetry.io/otel/exporters/prometheus`. (#5995)
- Fix attribute value truncation in
`go.opentelemetry.io/otel/sdk/trace`. (#5997)
- Fix attribute value truncation in `go.opentelemetry.io/otel/sdk/log`.
(#6032)
@MrAlias MrAlias mentioned this pull request Sep 30, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmathieu dmathieu dmathieu approved these changes
@dashpole dashpole dashpole approved these changes
@pellared pellared pellared approved these changes
@MrAlias MrAlias Awaiting requested review from MrAlias MrAlias is a code owner
@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.33.0

Development

Successfully merging this pull request may close these issues.

Add experimental Instrument.Enabled SDK method

4 participants

@codeboten @pellared @dmathieu @dashpole