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

feat(sdk-logs): emit resource attributes from ConsoleLogRecordExporter #4646

Merged

Conversation

harelmo-lumigo
Copy link
Contributor

Which problem is this PR solving?

Logging via the SDK emits the resource attributes, but ConsoleLogRecordExporter does not reflect that - which is very confusing and makes it hard to troubleshoot logging / resource-detection issues.
This PR adds the log-record's resource to the output that's logged to the console to aid troubleshooting.

Fixes #4645

Short description of the changes

Adding a new resource attribute to each log record emitted by ConsoleLogRecordExporter

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

By extending the current tests for ConsoleLogRecordExporter

@harelmo-lumigo harelmo-lumigo requested a review from a team as a code owner April 18, 2024 12:13
Copy link

linux-foundation-easycla bot commented Apr 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@hectorhdzg
Copy link
Member

Well my main concern is the same mentioned for Spans by @Flarna in #4605 (comment), this could be particularly bad and noisy in Logs, as this kind of telemetry could be way bigger in volume than spans, having this configurable would be ideal in my opinion.

@harelmo-lumigo
Copy link
Contributor Author

@hectorhdzg thanks for the feedback.

Just to make sure I got it, by:

having this configurable would be ideal in my opinion.

you mean introducing a new env-var to control the inclusion of resource attributes in each log? or a config-option in ConsoleLogRecordExporter's constructor?

@pichlermarc
Copy link
Member

@harelmo-lumigo I'd be inclined to use the same approach as for the trace exporter (keeping your PR as-is) here and see if the increased verbosity poses an actual problem for users. We should not go for differing console exporter interfaces unless necessary (like it is with the metrics exporter where we need temporality configuration for debugging purposes).

If we see that it's a problem and users are irritated by it, I'll open a PR to introduce the option that @trentm suggested in #4645 - we can then continue the discussion on that PR.

CHANGELOG.md Outdated Show resolved Hide resolved
@pichlermarc
Copy link
Member

@harelmo-lumigo thanks for working on this. To proceed, please follow the instructions on #4646 (comment) to sign the CLA 🙂

@hectorhdzg
Copy link
Member

@pichlermarc the way I see ConsoleExporters to be used are for local debugging for developers, when usually you don't have a lot of data flowing so this make perfect sense, but this is also being used as a self troubleshooting mechanism for OPs, where they can enable console exporters through environment variables and look at the actual real data of their services or applications, adding the resource information, that can be huge by the way compared to the actual log record, would make really hard for latest kind of usage. If we don't believe console exporters could be used in that kind of scenario then we should be good though.

@trentm
Copy link
Contributor

trentm commented Apr 24, 2024

If we see that it's a problem and users are irritated by it, I'll open a PR to introduce the option that @trentm suggested in #4645 - we can then continue the discussion on that PR.

Rather this comment: #4605 (comment)
The "ibid" thing might be similarly confusing, however. Not sure.

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

Ok I can see there was a long conversation in other PR mentioning exactly my concern, I was just worried this was not considered, I'm fine moving forward with this for now.

@pichlermarc
Copy link
Member

pichlermarc commented Apr 26, 2024

Thanks 🙂

Sorry for linking the wrong conversation earlier - as said before I'll continue to monitor user-concerns about having the resource there.

The situations I run into with users (4 times over since September, from actual support-tickets or directly reached out to me via slack while working on a POC) were:

  • users think the resource is not attached as they don't see it in the console exporter -> it turns out that everything is actually okay
  • users miss a specific resource attribute from detectors and they have no way to troubleshoot it by using a console exporter.

If you get any user feedback regarding this being too verbose please feel free to forward it to me - I'm happy to consider other alternatives but I wanted to go for the most obvious solution first. 🙂

@pichlermarc pichlermarc merged commit e44895f into open-telemetry:main Apr 26, 2024
18 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.

Logs emitted by LoggerProvider are missing the resource attributes
4 participants