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

chore: Use span fields as metrics labels #3888

Merged
merged 15 commits into from
Sep 26, 2020
Merged

chore: Use span fields as metrics labels #3888

merged 15 commits into from
Sep 26, 2020

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Sep 15, 2020

Closes #2660

Commits are semantic, should be easy to follow along through history.

@MOZGIII MOZGIII requested a review from bruceg September 15, 2020 15:00
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
src/metrics.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

This overall looks good!

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Sep 16, 2020

metrics-rs/metrics#87 has been merged, will switch the crate at Cargo.toml after new 0.13-alphas ship!

Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Copy link
Contributor Author

MOZGIII commented Sep 17, 2020

I found out that the notation we're using at topology at spawn_sink, spawn_transform and spawn_source injects the name and type as span data. I presume this is what we want to capture. One downside there is that names are a bit too generic, and can be easily overwritten in inner spans or logging calls.
I suppose this on it's own is not a good idea, and we should probably name them a bit more distinctively.

Also, the code I have filters by component_name annotation, but we can easily add more fields. We can't check the span name currently (i.e. to only grab the data from the sink/transform/source) - it's doable, but it doesn't seem like a good approach in the first place because it doesn't help with potential overrides at logging calls caused by inner spans or literal arguments at callsite. We probably want to just solve all those at once via specifying different span field names.

CC @lukesteensen

@binarylogic
Copy link
Contributor

I don't mind either way, but I would choose component_id and component_type for the context names.

Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

Looks okay, though the title really underplays what is going on here as it looks like a significant rework of our internal metrics underlay.

src/kubernetes/state/instrumenting.rs Outdated Show resolved Hide resolved
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
…tion

Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: MOZGIII <mike-n@narod.ru>
@MOZGIII
Copy link
Contributor Author

MOZGIII commented Sep 25, 2020

I don't mind either way, but I would choose component_id and component_type for the context names.

Ended up using topology_component_name and topology_component_type, because the component_type is already used as a literal value at the the emit!s.

Did a test run - it works great in practice. I suggest interested parties take the final look. I'll probably merge this on Monday.

@binarylogic
Copy link
Contributor

binarylogic commented Sep 26, 2020

Yeah, I'd prefer that we use component_name and component_type if possible. Can we not fix that? Not a huge deal if not.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Sep 26, 2020

@binarylogic I’d prefer that we don’t define the same field in two locations to eliminate the confusion with where they come from. So, I suggest removing the literals component_type from the emit! invocations from each of the components.
Alternatively, we can remove the component_type from the toolpology.

Not removing any would lead to metrics having two component_type labels with potentially different values (in the case where source is composed using other source/transform for instance).

@lukesteensen what do you think?

@binarylogic
Copy link
Contributor

It's fine. You've got 2 approvals, let's merge.

@MOZGIII MOZGIII merged commit 532fd30 into master Sep 26, 2020
@MOZGIII MOZGIII deleted the metrics-span-labels branch September 26, 2020 23:37
@lukesteensen
Copy link
Member

I suggest removing the literals component_type from the emit! invocations from each of the components.

Yes, my assumption was that we'd remove the manual tags from the event definitions once we're able to derive them from the span data. This is especially important for events that are emitted from shared components, where we previously couldn't know the exact type statically.

@MOZGIII
Copy link
Contributor Author

MOZGIII commented Sep 27, 2020

I'll create a follow-up PR to put this in order.

mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
* Update the metrics crate to 0.13.0-alpha

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Enable capturing tracing spans as metric labels

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Implement label filtering

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Add a test for label injection

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Add a workaround for disabled spans in tests

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Switch to published crates

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Fix clippy offences

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Fix the autoformatting

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Rename the ComponentNameFilter to VectorLabelFilter

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Actually use the VectorLabelFilter for TracingContextLayer initialization

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Adjust the actual spans and included label names

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Use the highest span level to avoid being affected by the log levels

Signed-off-by: MOZGIII <mike-n@narod.ru>

* Correct the metrics initialization at k8s instrumenting state tests

Signed-off-by: MOZGIII <mike-n@narod.ru>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include internal trace context with internal metrics
5 participants