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

[lokiexporter] Add Resource Attributes as Loki Label #3418

Merged
merged 14 commits into from
Jun 16, 2021

Conversation

eroteme
Copy link
Contributor

@eroteme eroteme commented May 18, 2021

Description:
Logrecord attributes usually only give information at the log level (such a severity and request url, etc).
Resource attributes usually add the context of what resource sent the log (such as container_name or cluster_name etc).

Currently the loki exporter is able to add logrecord attributes to loki labels. This PR adds Resource attributes also, which helps with filtering of loki data on both log attributes and resource attributes.

Link to tracking Issue:
#3405

Testing:
Added a few tests and a basic run of the exporter. I still have some questions about what happens if we have attributes with the same key on both log records and resources and which we would choose and how we deduplicate

Documentation:
Updated README.md, added code comments for doc creation

@eroteme eroteme requested a review from a team May 18, 2021 13:12
Fixes typo and used semantic conventions

Co-authored-by: gregoryfranklin <45693481+gregoryfranklin@users.noreply.github.com>
@tigrannajaryan tigrannajaryan requested a review from gramidt May 19, 2021 19:31
@gramidt
Copy link
Member

gramidt commented May 19, 2021

Excellent work, @eroteme! From a quick pass everything looks good; however, I will allocate more time tomorrow to dive in and perform a deeper review.

What are your thoughts on how you would like to handle duplicate labels?

@gregoryfranklin
Copy link
Contributor

What are your thoughts on how you would like to handle duplicate labels?

For duplicate labels, it might be worth considering the prometheus exporter as prior-art?
Loki labels are the same as OpenMetrics labels. The spec defines how to convert OpenTelemetry attribute keys into OpenMetrics label keys, but doesn't mention merging keys from both Resource and LogRecord/Metric Attributes.
The prometheus exporter just overwrites the metric attributes with labels coming from the resource. This seems reasonable to me.

Although, given you are explicitly listing which labels come from the Resource and which labels come from the LogRecord in the config, I don't think this is a big deal since you would have to explicitly configure a duplicate label situation.
Duplicate labels are therefore only an issue if the lokiexporter were later updated to support converting all attributes to labels instead of a specific list.

@gramidt
Copy link
Member

gramidt commented May 20, 2021

What are your thoughts on how you would like to handle duplicate labels?

For duplicate labels, it might be worth considering the prometheus exporter as prior-art?
Loki labels are the same as OpenMetrics labels. The spec defines how to convert OpenTelemetry attribute keys into OpenMetrics label keys, but doesn't mention merging keys from both Resource and LogRecord/Metric Attributes.
The prometheus exporter just overwrites the metric attributes with labels coming from the resource. This seems reasonable to me.

Although, given you are explicitly listing which labels come from the Resource and which labels come from the LogRecord in the config, I don't think this is a big deal since you would have to explicitly configure a duplicate label situation.
Duplicate labels are therefore only an issue if the lokiexporter were later updated to support converting all attributes to labels instead of a specific list.

Thank you so much for the prompt response! I personally like the idea of following the Prometheus exporter and overwriting the log attributes with the resource attributes. Let's try to do that in this PR.

@eroteme
Copy link
Contributor Author

eroteme commented May 20, 2021

Thank you so much for the prompt response! I personally like the idea of following the Prometheus exporter and overwriting the log attributes with the resource attributes. Let's try to do that in this PR.

The prometheus model.labelset Merge function used
https://github.com/prometheus/common/blob/f45215cb5711443fffd211cab7671b268245c4a1/model/labelset.go#L118

just needed reversing in this case but I've added a comment to show that this is happening in the Merge

mergedAttributes = logRecordAttributes.Merge(resourceAttributes)

This should mean that the logrecord attributes are overwritten by the resource attributes

exporter/lokiexporter/config.go Outdated Show resolved Hide resolved
exporter/lokiexporter/exporter.go Outdated Show resolved Hide resolved
@eroteme
Copy link
Contributor Author

eroteme commented Jun 1, 2021

I've tried to remove as much duplicated code by passing the attributes into the get attribute functions. I also noticed that the lokiExporter was storing the attribute labels when they where accessible from the config anyway. Let me know if this is ok.

@gramidt
Copy link
Member

gramidt commented Jun 8, 2021

@eroteme - This looks good. Could you rebase from main?

@gramidt
Copy link
Member

gramidt commented Jun 10, 2021

@bogdandrutu @tigrannajaryan - Can you allow the pipeline to run? Since @eroteme is a first time contributor to this repository, GitHub workflows require a Maintainer to approve running the workflow.

@bogdandrutu
Copy link
Member

@gramidt pipeline ran, but not sure what to do, can you approve if ready to merge?

@tigrannajaryan tigrannajaryan merged commit 198781c into open-telemetry:main Jun 16, 2021
@gramidt
Copy link
Member

gramidt commented Jun 16, 2021

Thank you for your contribution, @eroteme!

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.

6 participants