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

Loki: Option to add derived fields based on labels #76162

Merged

Conversation

5cat
Copy link
Contributor

@5cat 5cat commented Oct 7, 2023

What is this feature?
Loki only derives fields by a regex matcher, this limits its usage when functions such as line_formatter is used on top of the logs.
Some users already have logs parsed in json or logfmt structure which are detected as fields in loki.
This pull request allows the mapping between detected fields values and derived values by matching the fields' names.
Currently the feature is behind lokiDerivedFieldsFromLabels feature toggle.

Who is this feature for?

This feature is for users who want to have a minimal readable log lines while still having the ability to jump to traces via derived fields.
This feature is for users who prefer to offload the extraction of fields in their queries rather than being limited to what is regex can do.

Which issue(s) does this PR fix?:

Fixes #73598

Special notes for your reviewer:

I did my best to pattern match my way through this, and I dont understand the full implications of my changes so I have added a feature toggle for this.
I did my best to maintain backward compatibility and avoid any refactoring, wanted to keep my changes minimal.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Loki only derives fields by a regex matcher, this limits its usage when functions such as `line_formatter` is used on
top of the logs.
Some users already have logs parsed in json or logfmt structure which are detected as fields in loki.
This pull request allows the mapping between detected fields values and derived values by matching the fields' names.
Currently the feature is behind `lokiEnableNameMatcherOption` feature toggle.
@5cat 5cat requested review from grafanabot and a team as code owners October 7, 2023 08:01
@5cat 5cat force-pushed the 73598-loki-derived-field-from-detected-fields branch from 8232c69 to 7c5ee01 Compare October 7, 2023 08:04
@svennergr
Copy link
Contributor

Hi @5cat,

thanks a lot for your contribution. We will look into this and provide feedback in the upcoming days.

While we do this, are you sure this PR will fix #73598?

Thanks,
@svennergr

@5cat
Copy link
Contributor Author

5cat commented Oct 9, 2023

Hey @svennergr, yes it fixes the issue.

For example, in the past if you did add line_format in your query, the regex method for deriving the fields will not work since it operates on the displayed line, which is already edited by the line_format function.
With this pull request, having loki parse the logs and detect the field (with functions such as json logfmt unpack etc) is enough.

I called this matching option "Name matcher", since it matches the fields based on their names. which can be enabled from the provisioned datasource.yml via the enableNameMatcher option (which i made sure it is backward compatible) assuming you have toggled lokiEnableNameMatcherOption feature in grafana.ini:

  - name: gdev-loki
    type: loki
    access: proxy
    url: http://localhost:3100
    jsonData:
      derivedFields:
        - name: "traceID"
          enableNameMatcher: true
          url: "$${__value.raw}"
          datasourceUid: gdev-tempo

Or in the UI like this
image

So you can have a query such as {namespace="monitoring"} | logfmt | traceID != '' | line_format '[{{.level}}]: {{.msg}}'

And get derived fields correctly setup with the link.
image

now users can add line_format function to nicely format their logs while at the same time enjoy the ability to quickly jump into traces.

maybe @mladedav or @ThommyH can further confirm my work fixes these issues #73598 grafana/loki#9209 grafana/loki#6614 grafana/loki#9209 because they are all the same.

@gtk-grafana
Copy link
Contributor

@svennergr can we help make progress on this one?

@jseiser
Copy link

jseiser commented Oct 23, 2023

Just wanted to chime in here, we just ran into this issue in our POC of replacing our linkerd Jaeger deployment, with Linkerd/Tempo, since the integration between Loki and Tempo would be a huge plus for us.

All of our logs come in structured, so while I can see the traceID in the logs, and i can copy/paste into tempo, we have no way of actually using the integration that would be its biggest feature.

@ivanahuckova
Copy link
Member

@jseiser you should be able to do this trough new feature in Grafana - correlations. Here is the documentation for it https://grafana.com/docs/grafana/latest/administration/correlations/

@JakeCooper
Copy link

@jseiser you should be able to do this trough new feature in Grafana - correlations. Here is the documentation for it https://grafana.com/docs/grafana/latest/administration/correlations/

I've tried to set this up but it doesn't seem to work in the way we'd want? We'd want to ingest JSON and be able to show a link on the JSON key

CleanShot 2023-11-06 at 21 25 29@2x

I've attempted to use the correlations to do that, but it seems to only go off fields, so I need to use a derived field anyways, which only works if the field is itself in the query

CleanShot 2023-11-06 at 21 27 36@2x

@JakeCooper
Copy link

@ivanahuckova ^^

@ivanahuckova
Copy link
Member

ivanahuckova commented Nov 7, 2023

only go off fields

It does off data frame fields. In query inspector, you can see data frame fields. And you can then use transformation on extracting the value. So if your field is in labels, you can choose that as field and use regex to capture the field you want

image

@svennergr
Copy link
Contributor

Hi @5cat and others,

I know this PR has been open for a long time. I just wanted to let you know that I will be looking into this in the next days.
Thank you for your patience and your contribution to Grafana!

@svennergr svennergr added the no-backport Skip backport of PR label Nov 10, 2023
@svennergr svennergr requested review from svennergr and removed request for a team and grafanabot November 10, 2023 15:29
@svennergr svennergr changed the title Plugin: Deriving fields by name from parsed loki logs Loki: Option to add derived fields based on labels Nov 14, 2023
@svennergr svennergr added this to the 10.4.x milestone Nov 14, 2023
@svennergr svennergr removed their request for review November 14, 2023 10:05
Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

Left couple of comments, but overall looks good! 👍

/>
}
>
<Select
Copy link
Member

Choose a reason for hiding this comment

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

Select is too small for the "Regex in log line" option
image

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think the UI would be better if we had Name Type Regex/Label - to make type and regex/label more connected.

const newDerivedFields = [...fields, { name: '', matcherRegex: '', urlDisplayLabel: '', url: '' }];
const newDerivedFields = [
...fields,
{ name: '', matcherRegex: '', urlDisplayLabel: '', url: '', enableNameMatcher: false },
Copy link
Member

Choose a reason for hiding this comment

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

Should this be labelMatcher: 'true'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure, because that would technically break the default from "regex" to "label" and it might not be what users expect?

Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

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

Nice job, LGTM! 🚀

Thank you so much for your contribution @5cat . This is going to be EXTREMLY useful for many users! Really appreciate your contribution. 🧡

@svennergr svennergr merged commit 53758ad into grafana:main Nov 14, 2023
14 checks passed
@svennergr
Copy link
Contributor

Also a big thank you from my side, @5cat. As you probably have noticed we decided to release this without a feature toggle, such that every Grafana user can make use of your great feature! Hope to see further contributions from you 😊

@5cat
Copy link
Contributor Author

5cat commented Nov 14, 2023

Thank you @svennergr and @ivanahuckova for your amazing support. I'm very happy and excited to use this feature.

@javiplx
Copy link

javiplx commented Dec 29, 2023

Would this work with loki's structured metadata or just with real labels?

@ivanahuckova
Copy link
Member

Would this work with loki's structured metadata or just with real labels?

Yes, this will work with loki's structured metadata.

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

Successfully merging this pull request may close these issues.

Loki line_format clashes with derived fields
8 participants