enhancement: decouple livestore from metrics-generator by register a new metrics service#6506
Conversation
…new metrics service
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aab7e49b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| service Metrics { | ||
| rpc SpanMetricsSummary(SpanMetricsSummaryRequest) returns (SpanMetricsSummaryResponse) {} | ||
| rpc QueryRange(QueryRangeRequest) returns (QueryRangeResponse) {} |
There was a problem hiding this comment.
Preserve Metrics.SpanMetricsSummary RPC compatibility
Removing SpanMetricsSummary from the Metrics service is a wire/API breaking change for any existing gRPC clients generated from the previous tempo.proto: calls to /tempopb.Metrics/SpanMetricsSummary will fail with UNIMPLEMENTED after upgrading the server, even though the commit’s functional goal (adding a separate metrics service registration for live-store query-range) does not require dropping this RPC. Keeping the RPC and returning an explicit unimplemented response where needed would avoid breaking mixed-version or external clients.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
tempopb.Metrics wasn’t registered anywhere before, so /tempopb.Metrics/SpanMetricsSummary was already effectively unavailable.
What this PR does:
Currently the LiveStore registers the MetricsGenerator service for serving metrics queries. We need to decouple it first so we can have two different services. One for the pushing spans via grpc to the metrics generator and another one for serving queries.
The Metrics proto service was not used anywhere so we are reusing it.
From:
To
This will be done in a two phase deployment:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]