Skip to content

feat: Add ability to rename metrics based on attributes #34927

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

Closed
wants to merge 1 commit into from

Conversation

skandragon
Copy link

Description:

This change allows metric names to be remapped based on attribute values.

The use case here is that the metric names come in and mix with all the other metric feeds one may have, making it harder to filter and limit searches. For example, bytes_received may occur in many different AWS services, as well as many other metric sources. In our ingestion of firehose metrics, we want to rename this to be aws.elb.bytes_received to be specific about what object I am looking at.

I acknowledge that this is not strictly necessary as I could add other filters to further separate this in queries, but that is messy, and should another metric be introduced I would have to find all those filters and update them.

Link to tracking Issue:

None created.

Testing:

Tests were added for all the new code.

This has been running in our production environment for several weeks now, ingesting customer data as well as our own internal metrics.

Documentation:

Description and example added to README.

@skandragon skandragon requested review from a team and fatsheep9146 August 29, 2024 19:52
@github-actions github-actions bot requested a review from Aneurysm9 August 29, 2024 19:53
@skandragon skandragon force-pushed the allow-empty-key branch 2 times, most recently from ddb36cf to 53b5c18 Compare August 30, 2024 20:07
@MovieStoreGuy
Copy link
Contributor

I would question if this transformation logic should be included into the receiver. Components should be focused and follow the unix approach to keep them modular.

I worry by adding this functionality directly into the firebase receiver will muddle that and make the direction unclear for this specific component.

@skandragon
Copy link
Author

I thought about the same thing. The issue is, how would this work without having a receiver -> attribute renamed (which needs to have this functionality added) -> forwarder, and then forwarder -> rest of chain.

This is because we might not want to have the same decorations applied to all metric names, from many different receivers.

It feels like this is a "local context is best to handle this specific case" thing.

@MovieStoreGuy
Copy link
Contributor

MovieStoreGuy commented Sep 4, 2024

I thought about the same thing. The issue is, how would this work without having a receiver -> attribute renamed (which needs to have this functionality added) -> forwarder, and then forwarder -> rest of chain.

It should be as simple as:

aws firehouse --> transform processor --> { rest of pipeline }

transform processor as reference. It should be enough to get the metrics transformations you need, while using existing components and direction that is supported by the community.

This is because we might not want to have the same decorations applied to all metric names, from many different receivers.

I suggest applying these as seperate pipelines then:

[pipeline firehose ingest]
  firehose --> transform --> forward

[pipeline otlp ingestion]
  otlp --> forward

[fan in ingestion]
  forward --> {data lake}

I know that isn't the strict pipeline definitions but that is how I would suggest solving it so you're not managing custom code and custom transformations.

It feels like this is a "local context is best to handle this specific case" thing.

Making a dedicated pipeline and using a forward connector feels like it would solve this.

@skandragon
Copy link
Author

What existing processor do you suggest I add this to, or should I create a brand new processor?

I still feel the receiver should emit reasonable metric names to avoid the "every receiver ends up needing its own custom pipeline to fit into the larger world" scenario.

In any case, I've added log receiving to this as well, and we are about to add receiving some cloud trail data as well, so I will rebase those changes off a branch, and add the renaming mess to my collector to see how it performs.

My local helm chart is going to cry, just a little.

@MovieStoreGuy
Copy link
Contributor

What existing processor do you suggest I add this to, or should I create a brand new processor?

Use the transform processor, it should be able to do all the transformations you need without cluttering the firehose receiver.

I still feel the receiver should emit reasonable metric names to avoid the "every receiver ends up needing its own custom pipeline to fit into the larger world" scenario.

This receiver simply takes things off firehose so it can be consumed within the pipeline, if the metrics names are not appropriate then it would be best to change them upstream from the producer or using one of the existing processors to modify them.

Don't get me wrong, managing a bunch of yaml files to make sure these are correctly handled can be pain and would nice to be codified to allow easier means of testing but the provided changes doesn't align with what a receiver is responsible for.

@skandragon
Copy link
Author

This receiver simply takes things off firehose so it can be consumed within the pipeline, if the metrics names are not appropriate then it would be best to change them upstream from the producer or using one of the existing processors to modify them.

I'll follow this path, but there isn't a way to rename AWS-generated metrics for things like RDS clusters and S3 buckets, so these metrics are out of my control. However, I can whack on "aws." in front of metrics as you suggest.

I'll submit my cloud trail and cloud watch logs in a separate PR after I untangle this change from the base.

@skandragon skandragon closed this Sep 12, 2024
@skandragon skandragon deleted the allow-empty-key branch September 12, 2024 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants