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

subscriber: structured span fields #6

Closed
hawkw opened this issue May 4, 2021 · 5 comments · Fixed by #26
Closed

subscriber: structured span fields #6

hawkw opened this issue May 4, 2021 · 5 comments · Fixed by #26
Assignees
Labels
S-feature Severity: feature. This is adding a new feature.

Comments

@hawkw
Copy link
Member

hawkw commented May 4, 2021

right now, we just capture all span fields as a string. we should probably fix this

@hawkw hawkw self-assigned this May 4, 2021
@hawkw hawkw added the S-feature Severity: feature. This is adding a new feature. label May 4, 2021
@zaharidichev
Copy link
Collaborator

@hawkw I can do that if it is fine.

@hawkw
Copy link
Member Author

hawkw commented May 11, 2021

@hawkw I can do that if it is fine.

sure! at one point, i started working on the protobuf additions to do this; if you like, i can push up that change and you can take it from there, although i can't promise whether or not that will end up being useful.

@hawkw
Copy link
Member Author

hawkw commented May 11, 2021

okay @zaharidichev here's the branch where i started working on this: https://github.com/tokio-rs/console/compare/eliza/structured?expand=1

feel free to pick it up from there if you like. one note is that i think it would be nice to be able to encode statically known fields as indices into the field set in a span's metadata, rather than as strings, on the wire. although we may want to save that for a follow-up change, it could be good to reserve the ability to add it to the protobuf later.

@zaharidichev
Copy link
Collaborator

makes sense, given it is early days, can we get rid of the string representation field in the proto message now or we want to keep it around?

@hawkw
Copy link
Member Author

hawkw commented May 12, 2021

it would be fine to remove the string field from the proto, i think, since we're not currently making any stability guarantees for the proto...feel free to get rid of it when you have structured fields working end to end

@hawkw hawkw closed this as completed in #26 May 25, 2021
hawkw pushed a commit that referenced this issue May 25, 2021
This PR adds structured fields to the wire format.

Fixes #6

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-feature Severity: feature. This is adding a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants