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

fix(profiling)!: ensure a single label per sample has key "local root span id" #88

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

morrisonlevi
Copy link
Contributor

@morrisonlevi morrisonlevi commented Jan 17, 2023

What does this PR do?

This enforces that only a single label on a sample may contain a key of "local root span id". This means a Profile::add can now return an error case where it wouldn't before if multiple labels with keys of "local root span id" are sent. This is inherited by the FFI equivalent function as well, which returns 0 for such cases.

Motivation

I had a conversation with Ivo in #80 and continued over Slack in which he convinced me the code would be overall simpler if we did this. Aside from being non-sensical to have multiple local root span id labels, it's also an implementation optimization.

Additional Notes

I like having Ivo on the team :)

How to test the change?

Upgrade, and run tests as normal. Unless you are sending multiple labels with keys of "local root span id", then this will behave the same.

@morrisonlevi morrisonlevi requested a review from a team as a code owner January 17, 2023 20:57
@morrisonlevi morrisonlevi added the profiling Relates to the profiling* modules. label Jan 17, 2023
@morrisonlevi morrisonlevi changed the title Cleanup handling of "local root span id" labels Enforce only a single label has a key of "local root span id" Jan 17, 2023
@morrisonlevi morrisonlevi changed the title Enforce only a single label has a key of "local root span id" Enforce only a single label per sample has a key of "local root span id" Jan 17, 2023
Copy link
Contributor

@gleocadie gleocadie left a comment

Choose a reason for hiding this comment

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

only a nit, otherwise LGTM

profiling/src/profile/mod.rs Outdated Show resolved Hide resolved
@morrisonlevi morrisonlevi changed the title Enforce only a single label per sample has a key of "local root span id" fix(profiling)!: enforce only a single label per sample has a key of "local root span id" Jan 18, 2023
@morrisonlevi morrisonlevi changed the title fix(profiling)!: enforce only a single label per sample has a key of "local root span id" fix(profiling)!: ensure a single label per sample has a key of "local root span id" Jan 18, 2023
@morrisonlevi morrisonlevi changed the title fix(profiling)!: ensure a single label per sample has a key of "local root span id" fix(profiling)!: ensure a single label per sample has key "local root span id" Jan 18, 2023
@morrisonlevi morrisonlevi merged commit a9725b9 into main Jan 18, 2023
@morrisonlevi morrisonlevi deleted the levi/cleanup-lrsi branch January 18, 2023 16:53
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

When we initially discussed it, I was thinking that the local_root_span_id_label_offset could be a local variable in ::extract_sample_labels, which would force serialization to still need to iterate the labels. Storing the label is probably a tiny improvement, although since we clone the labels they'd probably still be in the processor cache so it's probably a small one ;)

Either way, I like it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
profiling Relates to the profiling* modules.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants