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

Consider removing include_trace_context from "Get a Logger" API #3394

Closed
Tracked by #2911
tigrannajaryan opened this issue Apr 13, 2023 · 2 comments · Fixed by #3397
Closed
Tracked by #2911

Consider removing include_trace_context from "Get a Logger" API #3394

tigrannajaryan opened this issue Apr 13, 2023 · 2 comments · Fixed by #3397
Assignees
Labels
spec:logs Related to the specification/logs directory

Comments

@tigrannajaryan
Copy link
Member

include_trace_context parameter has been a source of confusion (see for example here).

I suggest to remove it for now. It is supposed to be an optional parameter, so it can be added later as an optional parameter after we understand its behavior and interaction with the rest of context functionality better. We have a precedent of adding optional parameters to Stable APIs in the past, so it is OK to do so after the Bridge API is declared Stable.

@tigrannajaryan tigrannajaryan added the spec:logs Related to the specification/logs directory label Apr 13, 2023
@tigrannajaryan tigrannajaryan changed the title Consider removing include_trace_context from Logger API Consider removing include_trace_context from "Get a Logger" API Apr 13, 2023
@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-logs-approvers @scheler @MSNev FYI.

@jack-berg
Copy link
Member

I think we should remove it.

If you look at the git history, the parameter was originally added as part of the event API in #2676. Note that we didn't have an explicit Context parameter at the time.

Later in #2941 we split out the events API from the logs API, but kept the include_trace_context parameter.

Later in #2927 we added an explicit Context parameter, and didn't do a good job of defining how that context parameter interacts with include_trace_context.

As mentioned here I don't see how include_trace_context is anything but syntactic sugar for explicitly setting Context to empty. And I can't see how it serves any purpose at all in languages that require explicit context injection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:logs Related to the specification/logs directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants