Skip to content

Code review changes for #1043 #1208

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SpencerTorres
Copy link
Collaborator

@SpencerTorres SpencerTorres commented Mar 26, 2025

Builds on changes in #1043

There were some large structural/organization changes to be made to the original PR, so I decided to solve these in this PR so we can get it merged asap.

Example of events showing:
events example

Major Changes

1. Changed the "events" and "links" prefix from a SelectedColumn to a string

The string prefix is used within the SQL generator. This makes the editor components work as intended, since a string prefix isn't a column. This also gets rid of that helper function, as well as the ColumnHint definitions.

OTel defaults have been updated/set for this field

2. Added "Use Flatten Nested" option to config + editor

There's a discussion on how to handle flatten_nested=1. While the updated query is said to work with both versions, I was unable to get it to run. The default OTel table (along with the default ClickHouse setting) uses flatten_nested=0. This means we have distinct columns such as Events.Attributes and Events.TraceId instead of just Events as a tuple.

Instead of trying to work with both with one query, I added a setting that toggles the generated logic for this expression.
The updated query is also split on to multiple lines in the code since it was veeeeery long.

OTel defaults have been updated/set for this field

3. Refactored TracesConfig component tests

There was a ridiculous amount of duplicated code in here (I think I wrote it 👀), but I simplified it so that it now reuses a function for applying default props.

Other changes

  • Sync'd with main, it was out of date.
  • Updated deprecated components such as VerticalGroup and HorizontalGroup to Stack
  • Switch component now has disabled prop (required for disabling flatten_nested checkbox if otel is enabled)
  • Marked Trace InstrumentationLibrary Name and Version columns as optional in the tooltip. These columns don't exist on our default OTel trace table, but they're available if users want them.

Let me know if anything else should be changed, or if you have any questions. Thanks!

@SpencerTorres
Copy link
Collaborator Author

@harryjph I appreciate your changes! I was still having trouble with flatten_nested=0, so I made it a config option so users can simply control the behavior they want. Considering 0 is our default case, this value is set to false by default. Let me know if anything else stands out in this PR.

Here's the diff between this branch and the original PR: diff (it still has some changes from main in it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants