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

Add throughput and SLO metrics in the tags and tag values endpoints #4148

Merged
merged 27 commits into from
Oct 15, 2024

Conversation

electron0zero
Copy link
Member

@electron0zero electron0zero commented Oct 1, 2024

What this PR does:

propagate the bytes read from the storage layer to the frontend, and use bytes read to compute throughout, and use that in SLO computation for metadata endpoints.

metadata SLO thresholds can be configured via metadata_slo config.

search:
   metadata_slo:
      duration_slo: 5s
      throughput_bytes_slo: 1.073741824e+09

we will also return the metrics in the response of all the metadata endpoints:

  • /search/tags
  • /v2/search/tags
  • /search/tag/<tagName>/values
  • /v2/search/tag/<tagName>/values

here is what the it looks like in the response:

{
   "<existing keys>": "<existing response>",
  "metrics": {
    "inspectedBytes": "630188"
  }
}

we return "metrics": {} when response is empty or only contains intrinsics

it will also expose these new label to existing metrics with op="metadata" label

  • total metadata queries counter
  • metadata queries within SLO counter
  • metadata queries throughput histogram
Metrics
# HELP tempo_query_frontend_bytes_processed_per_second Bytes processed per second in the query per tenant
# TYPE tempo_query_frontend_bytes_processed_per_second histogram
tempo_query_frontend_bytes_processed_per_second_bucket{op="metadata",tenant="single-tenant",le="<value>"}
tempo_query_frontend_bytes_processed_per_second_sum{op="metadata",tenant="single-tenant"}
tempo_query_frontend_bytes_processed_per_second_count{op="metadata",tenant="single-tenant"}

# HELP tempo_query_frontend_queries_total Total queries received per tenant.
# TYPE tempo_query_frontend_queries_total counter
tempo_query_frontend_queries_total{op="metadata",tenant="single-tenant"}

# HELP tempo_query_frontend_queries_within_slo_total Total Queries within SLO per tenant
# TYPE tempo_query_frontend_queries_within_slo_total counter
tempo_query_frontend_queries_within_slo_total{op="metadata",tenant="single-tenant"}

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

modules/frontend/config.go Outdated Show resolved Hide resolved
modules/frontend/tag_handlers.go Outdated Show resolved Hide resolved
modules/ingester/instance_search.go Show resolved Hide resolved
pkg/collector/metrics_collector.go Show resolved Hide resolved
@knylander-grafana
Copy link
Contributor

Looks like a lot of great work! Do we need to add docs for this?

Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

99% lgtm. Really just the one last question about log format, which is actually been important lately as we are making all these improves to the read path.

require.Greater(t, inspectedBytes, uint64(300))
}

type searchTagsV2Response struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale for using the custom types for this test? If we can use the real proto types it seems best.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was seeing json: cannot unmarshal string into Go struct field MetadataMetrics.metrics.inspectedBytes of type uint64, see this CI run

json response will marshal the metrics into a string, so using this searchTagsV2Response to unmarshal the metrics and assert them.

modules/ingester/instance_search.go Show resolved Hide resolved
modules/frontend/tag_handlers.go Outdated Show resolved Hide resolved
@electron0zero
Copy link
Member Author

electron0zero commented Oct 11, 2024

Looks like a lot of great work! Do we need to add docs for this?

I will update the config page and API docs, that's the only docs change required.

Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Approving the doc additions. Thank you for updating the docs!

@electron0zero electron0zero merged commit 327c964 into grafana:main Oct 15, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants