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

Make context available in log record processor onEmit #2927

Merged
merged 4 commits into from Nov 9, 2022

Conversation

jack-berg
Copy link
Member

Related to #2911.

Context is not available to LogRecordProcessor#onEmit implementations at the moment. The API says that when emitting log records you must be able to set Trace Context, not a full Context. Accessing context in LogRecordProcessor#onEmit is important for a variety of use cases, including extracting and baggage entries as attributes on log records.

Implementations have to rely on languages supporting implicit context, something like:

    SdkLoggerProvider.builder()
        .addLogRecordProcessor(
            logRecord -> {
              Baggage baggage = Baggage.fromContext(Context.current());
              baggage
                  .asMap()
                  .forEach(
                      (key, entry) ->
                          logRecord.setAttribute(AttributeKey.stringKey(key), entry.getValue()));
            });

If implicit context is unavailable (i.e the language doesn't support it or the caller set the context explicitly) then accessing context is not possible.

This PR proposes extending onEmit with an additional argument, Context.

An alternative I considered was making Context accessible on ReadWriteLogRecord. This is a worse design because it causes references to Context to have to be held on longer in BatchLogRecordProcessor implementations.

specification/logs/api.md Outdated Show resolved Hide resolved
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with a changelog entry.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM. Consistent with what we do in SpanProcessor.

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

5 participants