Skip to content
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

feat: add span.Warnings to cassandra schema #4217

Closed
wants to merge 1 commit into from

Conversation

rohankmr414
Copy link

@rohankmr414 rohankmr414 commented Feb 6, 2023

Which problem is this PR solving?

Short description of the changes

  • added Span.Warnings to Cassandra dbmodel.Span

@rohankmr414
Copy link
Author

@albertteoh do I also need to include some sort of migration for cassandra schema

Comment on lines +39 to +40
start_time, duration, tags, logs, refs, process, warnings)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`
Copy link
Member

Choose a reason for hiding this comment

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

This won't work because there is no warning field in the table schema.

Considering how much headache it is to add a new column (need to version schema, invent some backwards compatibility logic, etc.), perhaps a less disruptive solution would be to encode warnings in either tags or logs, with a magic string prefix for tag names, so that they can be recognized at read time and converted back to warnings.

Copy link
Member

Choose a reason for hiding this comment

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

For example, we could create tags like this:

  • $$span.warning.1 = {warning1}
  • $$span.warning.2 = {warning2}

Copy link
Contributor

@utsavoza utsavoza Mar 17, 2023

Choose a reason for hiding this comment

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

Should we be concerned about the length of the warning (string) we receive in the span? Or do we directly encode the entire warning as is?

Copy link
Member

Choose a reason for hiding this comment

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

warnings come from our code, they are not very long. And we generally do not have restrictions on tag value length.

Copy link
Member

Choose a reason for hiding this comment

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

Alternative implementation in #4313

@yurishkuro
Copy link
Member

Replaced by #4313

@yurishkuro yurishkuro closed this Mar 19, 2023
yurishkuro pushed a commit that referenced this pull request Mar 19, 2023
## Which problem is this PR solving?
- Resolves #705
- Resolves #704 

## Short description of the changes
- Instead of creating a separate column in the table, [we encode
warnings as tags, with a magic string
prefix](#4217 (comment))
so that they can be parsed and converted back to warnings during reads.

---------

Signed-off-by: Utsav Oza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants