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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Dec 6, 2024
Merged

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

merged 9 commits into from
Dec 6, 2024

Conversation

codeboten
Copy link
Contributor

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

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
Copy link

codecov bot commented Dec 4, 2024 •
edited
Loading

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
Copy link
Member

@pellared pellared left a comment •
edited
Loading

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.

sdk/metric/internal/x/x.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
sdk/metric/instrument.go Outdated Show resolved Hide resolved
sdk/metric/internal/x/README.md Outdated Show resolved Hide resolved
sdk/metric/internal/x/x.go Outdated Show resolved Hide resolved
sdk/metric/internal/x/x.go Outdated Show resolved Hide resolved
sdk/metric/meter_test.go Outdated Show resolved Hide resolved
sdk/metric/meter_test.go Outdated Show resolved Hide resolved
sdk/metric/instrument.go Outdated Show resolved Hide resolved
@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
Copy link
Member

@pellared pellared left a comment

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👍

sdk/metric/meter_test.go Outdated Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Dec 5, 2024 •
edited
Loading

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 😉

@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
Copy link
Contributor 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
32 checks passed
@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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@dashpole dashpole dashpole approved these changes

@dmathieu dmathieu dmathieu 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