[metrics] Materialize all metrics tags into top level columns#6237
[metrics] Materialize all metrics tags into top level columns#6237
Conversation
mattmkim
force-pushed
the
matthew.kim/metrics-wide-schema
branch
3 times, most recently
from
fd2f214 to
7621e22
Compare
mattmkim
changed the title
mattmkim
force-pushed
the
matthew.kim/metrics-wide-schema
branch
from
7621e22 to
1b50f21
Compare
mattmkim
force-pushed
the
matthew.kim/metrics-wide-schema
branch
from
1b50f21 to
0b642de
Compare
mattmkim
force-pushed
the
matthew.kim/metrics-wide-schema
branch
from
0b642de to
7a5979f
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eae799d9a
ℹ️ 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".
| let empty_batch = | ||
| RecordBatch::new_empty(self.processor.schema().arrow_schema().clone()); | ||
| RecordBatch::new_empty(std::sync::Arc::new(arrow::datatypes::Schema::empty())); |
There was a problem hiding this comment.
Constructing the fallback checkpoint batch with Schema::empty() causes a hard failure when force_commit is true and no valid docs were forwarded (for example, all docs in the raw batch failed parsing): the indexer still flushes this batch, and ParquetWriter::write_to_file now rejects it because required columns are missing. In that path, the packager exits with an error instead of forwarding the checkpoint, which can stall ingestion progress for that shard.
Useful? React with 👍 / 👎.
| for scope_metrics in resource_metrics.scope_metrics { | ||
| for metric in scope_metrics.metrics { | ||
| parse_metric( | ||
| &metric, | ||
| &service_name, | ||
| &resource_attributes, | ||
| &mut data_points, | ||
| )?; | ||
| parse_metric(&metric, &service_name, &mut data_points, &mut num_rejected); | ||
| } |
There was a problem hiding this comment.
Resource attributes are parsed but no longer propagated into per-point tags: after extracting service.name, metrics are parsed with only service_name, so dimensions that commonly live at resource scope (like env, region, host, custom resource tags) are dropped entirely. This is a data-loss regression versus the previous behavior and breaks filtering/grouping on those tags for OTLP senders that attach dimensions at the resource level.
Useful? React with 👍 / 👎.
1 participant
Description
This PR can be reviewed commit by commit.
Currently, we define a static schema for metrics data in parquet. This PR makes the schema dynamic - all tags are put into their own columns during ingestion. Incoming metrics points must have
"metric_name", "metric_type", "timestamp_secs", "value"as fields.Again, a lot of metrics parsing/arrow logic lives in
quickwit-opentelemetry, when it should not. We will refactor this, eventually :)How was this PR tested?
Describe how you tested this PR.