feat: serve service histograms from aggregated metrics #625
Conversation
|
@matyax I see 2 options
|
| const lineFilter = sceneGraph.lookupVariable(VAR_LINE_FILTER, this) as CustomVariable; | ||
| const labels = sceneGraph.lookupVariable(VAR_LABELS, this) as AdHocFiltersVariable; | ||
|
|
||
| this._subs.add( |
There was a problem hiding this comment.
is this the correct way to be tracking this state?
| private getVizPanel() { | ||
| const service = this.service(); | ||
| const queryString = this.hasSingleServiceSelector() | ||
| ? `sum by (${LEVEL_VARIABLE_VALUE}) (sum_over_time({__aggregated_metric__=\`${service}\`} | logfmt | unwrap count [$__auto]))` |
There was a problem hiding this comment.
the _aggregated_metric__ query is still getting fields injected as filter statements ie service_name="foo". I'm surprised that would happen since I'm not using the $LOG_STREAM_SELECTOR_EXPR. Where's that coming from, and is there a way to disable it in this case? Or do I need to include __service_name__ in the aggregated log line?
There was a problem hiding this comment.
We use service_name as an "AddHocVariable": https://github.com/grafana/explore-logs/blob/main/src/Components/IndexScene/IndexScene.tsx#L180-L189
The way scenes works, it will try to add all those variables as add hoc filters to every query run. We would need to change the variable type in order for this to work. As a workaround for your PR you could change
https://github.com/grafana/explore-logs/blob/main/src/Components/IndexScene/IndexScene.tsx#L182 to something like datasource: { type: 'fake' },.
With that you wouldn't see those service_name labels included, however it would break showing the values in the label dropdowns.
|
|
||
| this._subs.add( | ||
| fields.subscribeToState((newState, prevState) => { | ||
| if (newState.filters.length !== prevState.filters.length) { |
There was a problem hiding this comment.
Filters may have the same length but different contents. What it's usually done, which is "not great", is comparing serialized strings with JSON.parse().
There was a problem hiding this comment.
Helper function in #633 is being proposed as solution for this
| return false; | ||
| } | ||
|
|
||
| private service(): string { |
There was a problem hiding this comment.
| private service(): string { | |
| private serviceName(): string { |
or
| private service(): string { | |
| private getServiceName(): string { |
I think we need to do something like this, because otherwise the app will not work for older Loki versions. |
When we move to |
|
Agree. |
trevorwhitney
force-pushed
the
aggregated-metrics
branch
from
9829b67
to
b9e7af5
Compare
|
@trevorwhitney Do you mind if we clean up the frontend part of this PR or would you prefer that we add comments? |
|
Feel free to clean it up! Thank you |
trevorwhitney
force-pushed
the
aggregated-metrics
branch
from
aa84f71
to
9f68f1f
Compare
This depends on grafana/loki#13621. Once that is merged, this change allows us to query those aggregated metrics to serve the service landing page histograms.