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

docs: observability.md: clarify lines vs. entries #1738

Merged

Conversation

jgehrcke
Copy link
Contributor

What this PR does / why we need it:

I was reading https://github.com/grafana/loki/blob/master/docs/operations/observability.md#observing-loki (much appreciated, thank you!) and came across the metric loki_distributor_lines_received_total. I know a little bit about the anatomy of the underlying protobuf structures where a log entry can have more or less arbitrary text, including newline-separated lines of text.

I wondered: does this metric count entries or lines?

The code tells us that it is actually counting entries, not lines.

I understand that changing the metric name is not low-friction, but very much thought it's worth clarifying that in the docs so that we don't confuse readers that pay attention to this level of detail.

What do you think?

Which issue(s) this PR fixes:
no issue number

Special notes for your reviewer:
I have used VSCode to edit the Markdown here, and it is auto-formatting tables in quite a meaningful way. I'd understand though if that's not wanted. Let me know.

Checklist

  • Documentation added
  • Tests updated

@jgehrcke jgehrcke force-pushed the jp/docs-observe-lines-vs-entries branch from 2623989 to df71722 Compare February 25, 2020 09:56
@jgehrcke jgehrcke changed the title docs: observability.md: clarify lines vs. entries observe lines vs entries docs: observability.md: clarify lines vs. entries Feb 25, 2020
@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

Merging #1738 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1738      +/-   ##
==========================================
+ Coverage    64.2%   64.23%   +0.03%     
==========================================
  Files         121      121              
  Lines        9085     9085              
==========================================
+ Hits         5833     5836       +3     
+ Misses       2848     2846       -2     
+ Partials      404      403       -1
Impacted Files Coverage Δ
pkg/ingester/transfer.go 65% <0%> (-1.43%) ⬇️
pkg/promtail/targets/filetarget.go 70.55% <0%> (+1.84%) ⬆️
pkg/promtail/targets/tailer.go 75.86% <0%> (+2.29%) ⬆️

| `loki_distributor_ingester_appends_total` | Counter | The total number of batch appends sent to ingesters. |
| `loki_distributor_ingester_append_failures_total` | Counter | The total number of failed batch appends sent to ingesters. |
| `loki_distributor_bytes_received_total` | Counter | The total number of uncompressed bytes received per tenant. |
| `loki_distributor_lines_received_total` | Counter | The total number of log entries received per tenant (misleading name, as an entry can have more than one line of text). |
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of misleading I would just explain that an entry can contains multiple line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @cyriltovena -- I force-pushed with updated wording. Would appreciate if you could have another look. Thanks!

@jgehrcke jgehrcke force-pushed the jp/docs-observe-lines-vs-entries branch from df71722 to 48f9f59 Compare February 26, 2020 13:49
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

Thanks. !!!

@cyriltovena cyriltovena merged commit 644d4be into grafana:master Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants