-
Notifications
You must be signed in to change notification settings - Fork 69
Add support for schema key aliases in query engine Parsers #1725
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
base: main
Are you sure you want to change the base?
Add support for schema key aliases in query engine Parsers #1725
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1725 +/- ##
==========================================
+ Coverage 84.65% 84.66% +0.01%
==========================================
Files 502 502
Lines 148609 148757 +148
==========================================
+ Hits 125799 125945 +146
- Misses 22276 22278 +2
Partials 534 534
🚀 New features to boost your workflow:
|
rust/experimental/query_engine/parser-abstractions/src/parser.rs
Outdated
Show resolved
Hide resolved
rust/experimental/query_engine/parser-abstractions/src/parser.rs
Outdated
Show resolved
Hide resolved
rust/experimental/query_engine/parser-abstractions/src/parser.rs
Outdated
Show resolved
Hide resolved
| "SpanId" => self.span_id.is_some(), | ||
| "TraceFlags" => self.flags.is_some(), | ||
| "EventName" => self.event_name.is_some(), | ||
| "Attributes" | "attributes" => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just musing here, this is kind of bloaty repeating itself a lot. Anything you can think of to try and consolidate/satisfy DRY principle?
It would be great if the parser could normalize everything in the tree so this code only need worry about the "canonical_key" for lookup. It could in some cases but I don't think we could do it always. Because it is possible to get some dynamic lookup at runtime think source[Attributes['some_key']]. Probably rare but not impossible 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did add a normalize function and changed all of these mappings to the 'canonical` key. I agree there's still a lot of repetition here...
Draft PR to open discussion - The current
otlp-bridgefor therecordsetengine uses the OpenTelemetry log data model spec for its initial schema keys (Attributes,Timestamp,ObservedTimestamp,SeverityText, etc).However, many well-versed in the OpenTelemetry space may be more used to the snake case representation (
attributes,time_unix_nano,observed_time_unix_nano,severity_text, etc) from the proto representation.Do we have any significant risks if we plan to support both? Inspired by
severity_textreference in #1722, been on the back of my mind for a while.This is still somewhat incomplete, could need more wiring for user-provided aliases in bridge, but for the moment just doing it for known OpenTelemetry fields.