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

Refactored attribute limits to be controlled by the LoggerProvider #129

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

evanlauer1
Copy link
Collaborator

@evanlauer1 evanlauer1 commented Jun 11, 2024

Big Context

Logging Support is being added to hs-opentelemetry. Logging spec

Small (This PR) Context

LoggerProviders should control the AttributeLimits for Loggers. Previously, the entirety of LogAttributes was provided to the emitLogRecord function instead of passing the attributes via a HashMap Text AnyValue and AttributeLimits via a LoggerProvider accessed through the provided Logger. Relatedly, LogAttributes should be able to be changed after the creation of the Logger, but that will be added later.

Testing

  • stack build runs

Comment on lines +134 to +137
addAttributes
(loggerProviderAttributeLimits loggerProvider)
emptyAttributes
attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there anything about this that addresses the part of the spec that says this:

There SHOULD be a message printed in the SDK’s log to indicate to the user that an attribute was discarded due to such a limit. To prevent excessive logging, the message MUST be printed at most once per LogRecord (i.e., not per discarded attribute).

Copy link
Owner

Choose a reason for hiding this comment

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

This is actually something that we don't support on the tracing side (which says effectively the same thing). I wanted to support it on the tracing side using OTel logging, so it's a bit of an ouroboros 😆

I think enough of the logging interface is in place that we should be be able to support this use case now though.

Copy link
Collaborator Author

@evanlauer1 evanlauer1 Jul 2, 2024

Choose a reason for hiding this comment

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

This is addressed by the next downstream pull request in the stack. The change was to large to be reasonably contained in this PR.

@evanlauer1 evanlauer1 force-pushed the refactor-attribute-limits-to-logger-provider branch from 27fbd05 to bdfb7c1 Compare June 14, 2024 21:52
@evanlauer1 evanlauer1 force-pushed the refactor-attribute-limits-to-logger-provider branch from bdfb7c1 to 8e35e04 Compare June 21, 2024 17:46
@evanlauer1 evanlauer1 changed the base branch from add-logger-provider-to-create-loggers to main June 21, 2024 17:46
@evanlauer1 evanlauer1 marked this pull request as ready for review June 21, 2024 19:02
@evanlauer1 evanlauer1 merged commit 695a68e into main Jul 3, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants