Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add tempo-cli command: convert parquet-2-to-3 #2828

Merged
merged 7 commits into from
Aug 25, 2023

Conversation

joe-elliott
Copy link
Member

What this PR does:
Adds a CLI command to convert a vParquet2 file to a vParquet block. The ergonomics are weird, but I couldn't find a better way to do it.

I had to make a vParquet2 method public to pull this off. Again, I'm not sure if this is possible otherwise. WDYT?

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding doc updates! Two minor suggestions for consistency.

@knylander-grafana knylander-grafana added the type/docs Improvements or additions to documentation label Aug 22, 2023
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that it's limited to span attributes.Using Tempo configs is the standard way of doing this in the tempo-cli. What do you think of that option?

joe-elliott and others added 3 commits August 25, 2023 07:54
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
Co-authored-by: Kim Nylander <104772500+knylander-grafana@users.noreply.github.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Member Author

@mapno

I don't like that it's limited to span attributes.Using Tempo configs is the standard way of doing this in the tempo-cli. What do you think of that option?

Added support for both resource and span columns. The thing I am most concerned about in this PR is exporting the vparquet2.ParquetTraceToTempopbTrace method.

Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joe-elliott joe-elliott merged commit 80a54c7 into grafana:main Aug 25, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants