Skip to content

Move exemplar types to non-internal package#5747

Merged
MrAlias merged 14 commits intoopen-telemetry:mainfrom
dashpole:public_exemplar
Sep 26, 2024
Merged

Move exemplar types to non-internal package#5747
MrAlias merged 14 commits intoopen-telemetry:mainfrom
dashpole:public_exemplar

Conversation

@dashpole
Copy link
Copy Markdown
Contributor

@dashpole dashpole commented Aug 27, 2024 •
edited
Loading

Part of #5249

This makes all existing types designed to implement the public Exemplar API public by moving most of internal/exemplar to exemplar. The only types that are not being made public are exemplar.Drop, and exemplar.FilteredReservoir. Those types are moved to internal/aggregate, and are renamed to DropReservoir and FilteredExemplarReservoir.

The following types are made public:

  • exemplar.Exemplar
  • exemplar.Filter
  • exemplar.SampledFilter
  • exemplar.AlwaysOnFilter
  • exemplar.HistogramReservoir
  • exemplar.FixedSizeReservoir
  • exemplar.Reservoir
  • exemplar.Value
  • exemplar.ValueType

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 27, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (6edc7a6) to head (204b29d).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5747   +/-   ##
=====================================
  Coverage   84.5%   84.5%           
=====================================
  Files        272     272           
  Lines      22734   22734           
=====================================
+ Hits       19226   19228    +2     
+ Misses      3165    3163    -2     
  Partials     343     343

see 21 files with indirect coverage changes

@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Sep 4, 2024

The README link doesn't work yet because the package isn't released. I assume I should just leave it there anyways?

dashpole
dashpole commented Sep 5, 2024
View reviewed changes
Comment thread sdk/metric/exemplar/README.md Outdated
dashpole
dashpole commented Sep 6, 2024
View reviewed changes
Comment thread sdk/metric/exemplar/README.md Outdated
@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Sep 19, 2024

@MrAlias rebased

XSAM
XSAM approved these changes Sep 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

👍

@MrAlias
Copy link
Copy Markdown
Contributor

MrAlias commented Sep 19, 2024 •
edited
Loading

Should we rename FixedSize and Histogram to FixedSizeReservoir and HistogramReservoir? Or is it enough, based on the function signature, that a user can figure out what it returns?

@XSAM
Copy link
Copy Markdown
Member

XSAM commented Sep 19, 2024

Should we rename FixedSize and Histogram to FixedSizeReservoir and HistogramReservoir?

I vote for this.

@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Sep 20, 2024

Renamed. I also took the liberty to rename the files to fixed_size_reservoir.go and histogram_reservoir.go while we are renaming things...

@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Sep 20, 2024

Possibly a follow-up question, but should we name these NewFixedSizeReservoir and NewHistogramReservoir, and export concrete types FixedSizeReservoir and HistogramReservoir? I can't think of why we would need methods on those other than implementing the Reservoir interface, but it is a generally good practice to accept interfaces and return concrete types.

@dashpole dashpole requested a review from XSAM September 20, 2024 15:48
@XSAM
Copy link
Copy Markdown
Member

XSAM commented Sep 20, 2024

should we name these NewFixedSizeReservoir and NewHistogramReservoir

Looks good.

export concrete types FixedSizeReservoir and HistogramReservoir

I don't have opinions on this.

@MrAlias
Copy link
Copy Markdown
Contributor

MrAlias commented Sep 20, 2024

Possibly a follow-up question, but should we name these NewFixedSizeReservoir and NewHistogramReservoir, and export concrete types FixedSizeReservoir and HistogramReservoir? I can't think of why we would need methods on those other than implementing the Reservoir interface, but it is a generally good practice to accept interfaces and return concrete types.

Both of these ideas sounds good to me, 👍

@dashpole dashpole force-pushed the public_exemplar branch 3 times, most recently from 39aa6f7 to 706e4ff Compare September 21, 2024 17:41
@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Sep 23, 2024

@MrAlias i'm waiting for you review before merging. Let me know if I should merge without your approval.

MrAlias
MrAlias reviewed Sep 26, 2024
View reviewed changes
Comment thread CHANGELOG.md Outdated
Comment thread sdk/metric/exemplar/value.go
Comment thread sdk/metric/exemplar/fixed_size_reservoir.go Outdated
@dashpole dashpole requested a review from MrAlias September 26, 2024 18:58
@MrAlias MrAlias merged commit 481f498 into open-telemetry:main Sep 26, 2024
@MrAlias
Copy link
Copy Markdown
Contributor

MrAlias commented Sep 26, 2024

Planned follow up discussed in SIG meeting: Audit the compliance of what was just merged with the specification.

TODO: track ^ in an issue.

@dashpole dashpole deleted the public_exemplar branch September 26, 2024 20:26
@pellared
Copy link
Copy Markdown
Member

pellared commented Sep 30, 2024

LGTM. Should we add some testable example showcasing exemplar-reservoir feature?

@MrAlias MrAlias added this to the v1.31.0 milestone Oct 10, 2024
@dashpole dashpole mentioned this pull request Oct 11, 2024
Merged
dashpole added a commit that referenced this pull request Oct 11, 2024
@dashpole
Release v1.31.0/v0.53.0/v0.7.0/v0.0.10 (#5883)
bc2fe88 
### Added

- Add `go.opentelemetry.io/otel/sdk/metric/exemplar` package which
includes `Exemplar`, `Filter`, `TraceBasedFilter`, `AlwaysOnFilter`,
`HistogramReservoir`, `FixedSizeReservoir`, `Reservoir`, `Value` and
`ValueType` types. These will be used for configuring the exemplar
reservoir for the metrics sdk. (#5747, #5862)
- Add `WithExportBufferSize` option to log batch processor.(#5877)

### Changed

- Enable exemplars by default in `go.opentelemetry.io/otel/sdk/metric`.
Exemplars can be disabled by setting
`OTEL_METRICS_EXEMPLAR_FILTER=always_off` (#5778)
- `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly
introduced `EnabledParameters` type instead of `Record`. (#5791)
- `FilterProcessor.Enabled` in
`go.opentelemetry.io/otel/sdk/log/internal/x` now accepts
`EnabledParameters` instead of `Record`. (#5791)
- The `Record` type in `go.opentelemetry.io/otel/log` is no longer
comparable. (#5847)
- Performance improvements for the trace SDK `SetAttributes` method in
`Span`. (#5864)
- Reduce memory allocations for the `Event` and `Link` lists in `Span`.
(#5858)
- Performance improvements for the trace SDK `AddEvent`, `AddLink`,
`RecordError` and `End` methods in `Span`. (#5874)

### Deprecated

- Deprecate all examples under `go.opentelemetry.io/otel/example` as
they are moved to [Contrib
repository](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/examples).
(#5854)

### Fixed

- The race condition for multiple `FixedSize` exemplar reservoirs
identified in #5814 is resolved. (#5819)
- Fix log records duplication in case of heterogeneous resource
attributes by correctly mapping each log record to it's resource and
scope. (#5803)
- Fix timer channel drain to avoid hanging on Go 1.23. (#5868)
- Fix delegation for global meter providers, and panic when calling
otel.SetMeterProvider. (#5827)
- Change the `reflect.TypeOf` to use a nil pointer to not allocate on
the heap unless necessary. (#5827)
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

@MrAlias MrAlias MrAlias approved these changes

@XSAM XSAM XSAM approved these changes

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@pellared pellared Awaiting requested review from pellared pellared is a code owner

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.31.0

Development

Successfully merging this pull request may close these issues.

5 participants

@dashpole @MrAlias @XSAM @pellared @dmathieu