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

[exporter/loki] Clarification about the hardcoded loki label "exporter=OTLP" #22155

Closed
gillg opened this issue May 22, 2023 · 10 comments
Closed

Comments

@gillg
Copy link
Contributor

gillg commented May 22, 2023

Component(s)

exporter/loki, pkg/translator/loki

Describe the issue you're reporting

Hello,

Working on a migration to the the recent Loki exporter, based on the pkg translator loki, which replace the initial version with some breaking changes, I'm wondering if the hardcoded label exporter=OTLP is not a conventional mistake.

OTLP is the OpenTelemetry Protocol, but by using the Loki exporter I don't understand what that means.
Your source logs can come from many source, not necessarly OTLP, and the otel collector doesn't use the OTLP protocol to send logs to Loki so I don't understand. I would eventualy hardcode a label "exporter=OtelCollector" or something else, but OTLP is probably a conceptual mistake.

I'm also wondering if it makes completely sense to hardcode a label like that https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/loki/convert.go#LL31C22-L31C22
Maybe the end user don't want to have a common arbitrary label to all the logs ?
It's not a problem in my case, but it seems not very flexible.

@gillg gillg added the needs triage New item requiring triage label May 22, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@mar4uk
Copy link
Contributor

mar4uk commented May 22, 2023

To send a log entry to Loki at least one label is needed. It was decided to set default label {exporter=OTLP}, so users can see their logs in loki even if they didn't configure any labels themselves. Log entry without at least one label would be dropped by loki.

I think it would be nice to allow users to configure setting default labels (there is a discussion about it #21508). Will work on it soon

@gillg
Copy link
Contributor Author

gillg commented May 22, 2023

I understand the initial wish to avoid dropped logs.
My main concern is the hardcoded value "OTLP" which is missleading because there is no clear reason to use this term here.

But definitely agreed, being able to select the default labels to add or not is a nice enhancement.

@mar4uk
Copy link
Contributor

mar4uk commented May 23, 2023

exporter=OTLP means that the log was produced from the OpenTelemetry Logs Data Model.
We can use the same label for otlp exporter as well and Loki will know what kind of logs it deals with.
I would keep this label as it is for now

@jpkrohling
Copy link
Member

My main concern is the hardcoded value "OTLP" which is missleading because there is no clear reason to use this term here.

I'll take the blame for this, it should have been "otelcol" or something like that. The label was originally proposed by @cyriltovena when we were doing a refactoring of this component a year or so ago, but I should have proposed something better than OTLP. At this point, changing this would likely cause confusion to current users and I would propose the exporter to have a "default label/value" property, with the possibility of this being disabled. Like:

defaults:
  label:
    enabled: true # default to true, meaning that all data points being exported will have the default label/value
    name: exporter # default to  "exporter", which is the current label name
    value: OTLP # default to "OTLP", which is the current label value

Not having a "defaults" node or "label" node keeps the current behavior. Setting "enabled" to false would disable the default label entirely, meaning that users should ensure that labels must exist for log entries to be submitted to Loki. We should also have a metric to record the number of log entries that got ignored because they lacked a label.

@gillg
Copy link
Contributor Author

gillg commented May 23, 2023

Thaks for the details @jpkrohling ! Understood

So I agree, with a way to disable or configure the hardcoded values.
That cross the other proposal issue above opened by @mar4uk

About the metrics I thought the absence of labels would be monitored by otelcol_exporter_send_failed_log_records{exporter="loki"}. It's not the case ?

@jpkrohling
Copy link
Member

That would be one case, but it would store all failures, not only the ones caused by not having any labels in the record.

@andrzej-stencel andrzej-stencel removed the needs triage New item requiring triage label Jun 29, 2023
@mar4uk
Copy link
Contributor

mar4uk commented Jul 27, 2023

as part of #23863 I added new metric otelcol_lokiexporter_send_failed_due_to_missing_labels to keep track of records dropped because of no labels were set

@mar4uk
Copy link
Contributor

mar4uk commented Aug 4, 2023

This issue was addressed by #23863
Now we can disable default labels
To override the default value of exporter=OTLP label we can use attributes processor

@jpkrohling
Copy link
Member

If you believe there's still anything pending for this, feel free to reopen or comment here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants