feat: persist patterns as aggregated metrics #17737
Conversation
trevorwhitney
force-pushed
the
persist-patterns-as-aggs
branch
from
0327353 to
60eb96d
Compare
|
💻 Deploy preview available: |
|
It feels kind of wrong to me that we are reusing aggregated_metric naming and the aggregated_metric label for this as it's not an aggregated metric. I think we should use a different label for this? And maybe there is a different name we could use for code that is shared which isn't specific to aggregated metrics and usable for both |
| # CLI flag: -pattern-ingester.metric-aggregation.downsample-period | ||
| [downsample_period: <duration> | default = 10s] | ||
| # Configures the aggregation and storage behavior of the pattern ingester. | ||
| aggregation: |
There was a problem hiding this comment.
i wonder if rather than making this generic, we should have separate configs for metric_aggregation and pattern persistence.
I think we are overloading concepts here in a way that is a bit confusing and potentially harder to optimize in the future as we replace our storage (a different label identifier for these types would be important for handling them differently if we choose to in the future)
| @@ -144,37 +144,37 @@ func Test_Push(t *testing.T) { | |||
|
|
|||
| p.WriteEntry( | |||
| wayBack, | |||
| AggregatedMetricEntry(model.TimeFromUnix(wayBack.Unix()), 1, 1, "test_service", lbls1), | |||
| AggregatedMetricEntry(model.TimeFromUnix(wayBack.Unix()), 1, 1, lbls1), | |||
There was a problem hiding this comment.
If this is re-usable between aggregated metrics and patterns, maybe a better name would be "MetricEntry"?
| } | ||
|
|
||
| newLbls := labels.Labels{ | ||
| labels.Label{Name: constants.AggregatedMetricLabel, Value: service}, |
There was a problem hiding this comment.
I think this should use a new label name, aggregated metrics and pattern persistence are quite different things and it feels wrong to me to store patterns in a stream called "aggregated metrics" also feels like it would be less performant at query time because you can't differentiate them when you query.
|
@slim-bean and I talked about his feedback. my motivation for putting these in the same stream as aggregated metrics was to co-locate as much of the aggregated data together as I could to hedge against having lots of really small chunks. given the move to dataobjs that is currently under way, however, @slim-bean pointed out this shouldn't be a problem for long, and the logical/naming separation is worth the trade off, so I'm moving forward with that refactor. |
What this PR does / why we need it:
This PR is the first step to enable persistence of patterns in Logs Drilldown beyond 3h. It persists the patterns back to Loki in the
__aggregrated_metric__stream, similar to aggregated metrics. This seems like the best solution for now, with the eventual goal of putting this type of information in dataobjs.Screenshot of comparing these patterns to the existing.
This PR also changes how patterns are stored in order to capture level in addition to the stream selector. This allowed the patterns to be persisted with level, and is needed for an upcoming change to the UI to group and filter patterns by level.
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR