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] Refactor component, reducing configuration #12873

Closed
jpkrohling opened this issue Aug 1, 2022 · 5 comments · Fixed by #13572
Closed

[exporter/loki] Refactor component, reducing configuration #12873

jpkrohling opened this issue Aug 1, 2022 · 5 comments · Fixed by #13572
Assignees
Labels
exporter/loki Loki Exporter priority:p2 Medium

Comments

@jpkrohling
Copy link
Member

jpkrohling commented Aug 1, 2022

In preparation for having a possible OTLP endpoint in Loki to ingest logs, we need to refactor the Loki exporter to have minimal configuration, potentially with only the endpoint. All other configuration options would be hints to be provided as resource or regular attributes, like the following:

receivers:

exporters:
  loki:
    endpoint: https://localhost:3100/loki/api/v1/push

processors:
  attributes:
    actions:
    - action: insert
      key: loki.attribute.labels
      value: [host.name, pod.name]
    - action: insert
      key: loki.resource.labels
      value: [host.name, pod.name]

extensions:

service:
  extensions:
  pipelines:
    logs:
      receivers: []
      processors: [attributes]
      exporters: [loki]

We still have questions about the tenant information. We could either have a generic solution that would work for all HTTP and/or gRPC-based clients, or we could have a hint similar to the example above, like:

    - action: insert
      key: loki.tenant
      from_context: tenant

To accomplish this, we need to provide an initial PR deprecating the current options and translation to the new approach.

One option will be removed without replacement, namely the "format". We'd like to hear from you if you currently use this and cannot use the JSON format. If we don't hear a concrete reason why people need the "body" option, which is currently the default, we'll switch the default to "json" and remove the "format" option altogether.

The changes proposed here are needed because we'll need the same hints in the future when we add OTLP support for Loki. Making those changes now will make the migration path and future deprecation of the Loki exporter easier.

@gregoryfranklin
Copy link
Contributor

Loki only has a concept of labels, not resource labels and attribute labels.

So, would you not just want the key to be loki.labels?

processors:
  attributes:
    actions:
    - action: insert
      key: loki.labels
      from_attribute: [host.name, pod.name]

At the moment (as far as I'm aware), the attribute processor wouldn't meet the requirements in the current form - it does not support setting a key to a list of values, it does not support resource attributes and it cannot extract non-attribute fields such as severity.

In principle, however, this approach would work for me.

@TylerHelmuth TylerHelmuth added exporter/loki Loki Exporter priority:needed Triagers reviewed the issue but need code owner to set priority labels Aug 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022

Pinging code owners: @gramidt @jpkrohling. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@jpkrohling
Copy link
Member Author

So, would you not just want the key to be loki.labels?

The exporter would need to know whether a specific attribute is part of the resource or regular attributes. While we can parse both and return the most specific one (regular, when both regular and resource are present), allowing users to be explicit would reduce confusion and would allow for a small optimization on the lookup.

it does not support resource attributes

There's the resource processor for that.

it does not support setting a key to a list of values
it cannot extract non-attribute fields such as severity

I'll do some PoC, but wouldn't the transform processor also help here?

@a06953
Copy link

a06953 commented Aug 8, 2022

My issue was resolved by setting up the attribute. Thanks all

@jpkrohling
Copy link
Member Author

@gregoryfranklin, I just checked and you are right, the attributes processor does not support slices in its current form:

// NewAttributeValueRaw is used to convert the raw `value` from ActionKeyValue to the supported trace attribute values.
// If error different than nil the return value is invalid. Calling any functions on the invalid value will cause a panic.
func NewAttributeValueRaw(value interface{}) (pcommon.Value, error) {
switch val := value.(type) {
case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64:
return pcommon.NewValueInt(cast.ToInt64(val)), nil
case float32, float64:
return pcommon.NewValueDouble(cast.ToFloat64(val)), nil
case string:
return pcommon.NewValueString(val), nil
case bool:
return pcommon.NewValueBool(val), nil
default:
return pcommon.Value{}, fmt.Errorf("error unsupported value type \"%T\"", value)
}
}

However, pcommon seems to allow it:
https://github.com/open-telemetry/opentelemetry-collector/blob/c69aac5e9dc6e37d75bf96caef6c9e42a2b3ac55/pdata/pcommon/alias.go#L73-L74

I'll play a bit with it, but if this doesn't work, we could do something like:

processors:
  attributes:
    actions:
    - action: insert
      key: loki.attribute.labels
      value: "host.name, pod.name"

Not the best, but not too bad either. What do you think?

jpkrohling referenced this issue in jpkrohling/opentelemetry-collector-contrib Aug 31, 2022
This commit brings a new implementation of the Loki exporter while
maintaining compatibility with the previous version (called "legacy" in
this PR). The new exporter has a cleaner logic, allowing the Loki labels
to be configured as part of the data points themselves instead of being
part of the static configuration. This will be useful in the future when
migrating from the Loki exporter to a native OTLP endpoint in Loki.

Fixes #12873

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling added a commit that referenced this issue Sep 1, 2022
This commit brings a new implementation of the Loki exporter while
maintaining compatibility with the previous version (called "legacy" in
this PR). The new exporter has a cleaner logic, allowing the Loki labels
to be configured as part of the data points instead of being
part of the static configuration. This will be useful in the future when
migrating from the Loki exporter to a native OTLP endpoint in Loki.

Fixes #12873

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/loki Loki Exporter priority:p2 Medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants