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

LogQL: Improve template format #2822

Merged
merged 40 commits into from
Nov 6, 2020
Merged

Conversation

cyriltovena
Copy link
Contributor

This add errors in case of problem when executing template for line/label_format.

It also add functions that are easier to use with pipelines.

Docs & tests are updated.

@cyriltovena cyriltovena changed the title Improve template format LogQL: Improve template format Oct 27, 2020
@cyriltovena cyriltovena added this to To do in Loki Project Oct 27, 2020
@cyriltovena cyriltovena added 2.1 and removed 2.1 labels Oct 27, 2020
@cyriltovena cyriltovena added this to the 2.1 milestone Oct 27, 2020
@cyriltovena cyriltovena moved this from To do to In progress in Loki Project Oct 27, 2020
Copy link
Contributor

@oddlittlebird oddlittlebird left a comment

Choose a reason for hiding this comment

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

Some doc edits, mostly minor. I suggest having someone else review the code.

docs/sources/logql/_index.md Outdated Show resolved Hide resolved
docs/sources/logql/_index.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
Loki Project automation moved this from In progress to Review in progress Oct 27, 2020
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor nit.

pkg/logql/log/fmt.go Outdated Show resolved Hide resolved
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Good catch on the function param positions. We can probably deprecate the old ones officially in the future.

All the string indexing math should use []rune instead of string indices, because the latter doesn't play well with all code points. I've left a couple ideas about implementation, curious what you think.

pkg/logql/log/fmt.go Outdated Show resolved Hide resolved
pkg/logql/log/fmt.go Show resolved Hide resolved
pkg/logql/log/fmt.go Show resolved Hide resolved
cyriltovena and others added 16 commits November 5, 2020 13:06
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
cyriltovena and others added 3 commits November 5, 2020 13:06
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena requested review from oddlittlebird and removed request for achatterjee-grafana November 5, 2020 14:20
docs/sources/logql/_index.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Added a bunch of minor copy-edit suggestions.

docs/sources/logql/_index.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved

The [text template](https://golang.org/pkg/text/template) format used in `| line_format` and `| label_format` support functions the usage of functions.

All labels are added as variables in the template engine. They can be referenced using they label name prefixed by a `.`(e.g `.label_name`) for example the following template will output the value of the path label:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
All labels are added as variables in the template engine. They can be referenced using they label name prefixed by a `.`(e.g `.label_name`) for example the following template will output the value of the path label:
Labels are added as variables in the template engine. They can be referenced using the label name prefixed by a `.`(e.g. `.label_name`). The following template will output the value of the path label:

docs/sources/logql/functions.md Outdated Show resolved Hide resolved
`{{ToUpper "This is a string" | ToLower}}`
```

> **Note:** In Loki 2.1 you can also use respectively [`lower`](#lower) and [`upper`](#upper) shortcut, e.g `{{.request_method | lower }}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> **Note:** In Loki 2.1 you can also use respectively [`lower`](#lower) and [`upper`](#upper) shortcut, e.g `{{.request_method | lower }}`
> **Note:** In Loki 2.1 you can also respectively use [`lower`](#lower) and [`upper`](#upper) shortcut, e.g `{{.request_method | lower }}`

docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
docs/sources/logql/functions.md Outdated Show resolved Hide resolved
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Cool bits around negative truncation. Code LGTM (thanks for fixing rune support).

@cyriltovena
Copy link
Contributor Author

@oddlittlebird @achatterjee-grafana I think after 5 suggestions it would better to just add a commit to my PR on your own.

The result is the same, but for me it's less time wasted it's not easy to accept all those suggestions with github UI, but also remembering what I've solved or not.

WDYT ?

cyriltovena and others added 12 commits November 5, 2020 14:58
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Co-authored-by: achatterjee-grafana <70489351+achatterjee-grafana@users.noreply.github.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Copy link
Contributor Author

I wasn't able to accept all of them because I think some are overlapping on the same lines and they became outdated.

@oddlittlebird
Copy link
Contributor

@oddlittlebird @achatterjee-grafana I think after 5 suggestions it would better to just add a commit to my PR on your own.

The result is the same, but for me it's less time wasted it's not easy to accept all those suggestions with github UI, but also remembering what I've solved or not.

WDYT ?

No one has shown us how to do that, so theoretically maybe?

Would that still allow you (or users in general) to accept some suggestions and reject others?

@cyriltovena
Copy link
Contributor Author

@oddlittlebird @achatterjee-grafana I think after 5 suggestions it would better to just add a commit to my PR on your own.
The result is the same, but for me it's less time wasted it's not easy to accept all those suggestions with github UI, but also remembering what I've solved or not.
WDYT ?

No one has shown us how to do that, so theoretically maybe?

Would that still allow you (or users in general) to accept some suggestions and reject others?

I could technically revert a commit if really I wasn't happy about the changes, but honestly, you're the code owner of this, I fully trust you, specially for documentation.

I think when there's so many suggestions/mistakes/edits, it's fine to do that. (At least for me, I allow you to do so, I'll still learn from seeing the changes, but I won't waste time trying to get everything right, I was able to batch but it's only by 8.)

Now how to do it, might not be all simple if you're not familiar to git, I'll admit that.

However you might be able to do it directly in github using this edit button which should get you into an editor.

image

Or using git locally

  1. get github cli https://github.com/cli/cli
  2. gh auth login (first time to auth)
  3. gh pr checkout number (to get my changes locally)
  4. edit files
  5. git add .
  6. git commit -m "documentation changes"
  7. git push

I think this was a rare case of many changes required, so I don't think this is required for every PR.

Loki Project automation moved this from Review in progress to Reviewer approved Nov 5, 2020
Copy link
Contributor

@oddlittlebird oddlittlebird left a comment

Choose a reason for hiding this comment

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

Thanks for the lesson! I'll try the commit the next time I have a lot of changes to make.

@cyriltovena cyriltovena merged commit e2aee2a into grafana:master Nov 6, 2020
Loki Project automation moved this from Reviewer approved to Done Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants