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

log: Change EnabledParameters to have a field instead of getter and setter #6009

Merged
merged 9 commits into from
Dec 3, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Nov 29, 2024

What

Allow users to to write less verbose code by changing methods to field so that the callers can do:

if logger.Enabled(ctx, log.EnabledParameters{ Severity: log.SeverityDebug }) {
 // ...
}

instead of

params := log.EnabledParameters{}
params.SetSeverity(log.SeverityDebug)
if logger.Enabled(ctx, params) {
 // ...
}

Why

  • We may provide a Logger.EnabledEvent user-facing API in close future and it would be nice if the signatures are similar. See: Add Event Enabled operation to Logger opentelemetry-specification#4263. The possible alternative is that user-facing Logger.Enabled would also be used for events as well.

  • There is a chance that the Logger API is going to be user-facing. See: Define whether instrumentations can emit non-event log records opentelemetry-specification#4234.

  • I (subjectively) believe that having a shorter type name and using fields instead of methods has a better user experience.
    Ideally the user would prefer having Enabled(ctx context.Context, severity SeverityLevel) bool.
    However, we would be not able to provide additional parameters without creating new methods.

  • I was thinking for for a few weeks before proposing this PR. I was worried if fields would be able to handle all possible cases. However, we can always add methods for future parameters. Having mixed fields and methods should not be the end of the world - especially that it is e.g. how slog.Record is designed. We were discussing inside Logs SIG if we ever would like to add (partial) attributes or (partial) body to Logger.Enabled. We all agreed that we do not want it as it can introduce overhead that we want to avoid and also it can lead to a very confusing API experience. Moreover, event name is being promoted to a top-level field of a log record - this was the only "attribute" that seem to be important, but there are even more reasons to promote it to a top-level field. See: Add EventName to Log and Event Record in data model opentelemetry-specification#4260. Therefore, there is a pretty low chance that we will introduce any complex parameter in future.

@pellared pellared changed the title log: Change EnabledParameters to EnabledParams log: Change EnabledParameters to simple EnabledParams Nov 29, 2024
Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.1%. Comparing base (9f82e51) to head (581ea29).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6009   +/-   ##
=====================================
  Coverage   82.1%   82.1%           
=====================================
  Files        273     273           
  Lines      23648   23643    -5     
=====================================
- Hits       19433   19432    -1     
+ Misses      3870    3866    -4     
  Partials     345     345           

see 2 files with indirect coverage changes

@pellared pellared marked this pull request as ready for review November 29, 2024 13:12
@pellared pellared added pkg:API Related to an API package area:logs Part of OpenTelemetry logs labels Nov 29, 2024
@pellared pellared self-assigned this Nov 29, 2024
@dashpole
Copy link
Contributor

dashpole commented Dec 2, 2024

Do we have any other examples of FooParameters or FooParams? Consistency across the codebase is important to me

@MrAlias
Copy link
Contributor

MrAlias commented Dec 2, 2024

https://pkg.go.dev/go.opentelemetry.io/otel/sdk/trace#SamplingParameters

@pellared
Copy link
Member Author

pellared commented Dec 3, 2024

Do we have any other examples of FooParameters or FooParams? Consistency across the codebase is important to me

Renaming back so this PR would only propose to have fields instead of getters and setters.

@pellared pellared changed the title log: Change EnabledParameters to simple EnabledParams log: Change EnabledParameters to have a field instead of getter and setter Dec 3, 2024
@pellared
Copy link
Member Author

pellared commented Dec 3, 2024

@dashpole, @MrAlias, renamed back to EnabledParameters.

There is need to rush with this PR. We can discuss it during SIG meeting.

@dashpole
Copy link
Contributor

dashpole commented Dec 3, 2024

I'm definitely ok with this. Having a struct for sampling has worked well so far, and this seems similar.

log/logtest/recorder_test.go Outdated Show resolved Hide resolved
@pellared pellared requested a review from MrAlias December 3, 2024 16:26
@pellared pellared merged commit 2f0bf8e into open-telemetry:main Dec 3, 2024
30 checks passed
@pellared pellared deleted the enabled-params-fields branch December 3, 2024 17:37
@MrAlias MrAlias added this to the v1.33.0 milestone Dec 11, 2024
@MrAlias MrAlias mentioned this pull request Dec 12, 2024
MrAlias added a commit that referenced this pull request Dec 12, 2024
### 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
Labels
area:logs Part of OpenTelemetry logs pkg:API Related to an API package
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants