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

Add support for custom attributes function #3198

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

urdarinda
Copy link

@urdarinda urdarinda commented Nov 25, 2024

Sometimes we want to add custom attributes to metrics in order to give better context with the application.

Example,
I want to identify which API is responsible for calling this redis command and group by the calling API

@urdarinda urdarinda force-pushed the jatin/add-custom-attrs branch from 120cdd0 to 64bfb34 Compare November 25, 2024 18:28
@urdarinda
Copy link
Author

@ofekshenawa Please take a look at this one

@ofekshenawa
Copy link
Collaborator

Can you please add some tests?

@urdarinda
Copy link
Author

Can you please add some tests?

Added tests for metrics and custom attributes

@urdarinda urdarinda force-pushed the jatin/add-custom-attrs branch from 0524dee to b04e7ba Compare December 17, 2024 13:31
@urdarinda
Copy link
Author

@ofekshenawa Added tests but something weird is happening with the tests. It was passing tests yesterday and failing now. It should only fail on 1.19 afaik because the metric sdk version I am using requires 1.20 and above ( not able to find a working version before that)

@urdarinda urdarinda force-pushed the jatin/add-custom-attrs branch from fda6b07 to f7d3f48 Compare December 17, 2024 13:59
@urdarinda
Copy link
Author

@ofekshenawa Fixed the tests, but it is failing on 1.19 ( the test setup for metrics require 1.20+) , before those not able to find test functions for metric testing. How can we proceed?

@urdarinda
Copy link
Author

@ndyakov can you please take a look at this one?

Thanks

@ndyakov
Copy link
Collaborator

ndyakov commented Feb 3, 2025

@urdarinda I do not have a lot of context on the redisotel, but don't see a breaking change. Would you mind updating the example in the examples as well?

@urdarinda
Copy link
Author

@ndyakov added

@ndyakov
Copy link
Collaborator

ndyakov commented Mar 4, 2025

@urdarinda please resolve conflicts and request a review from @monkey92t since maybe he has more background with the otel package than me.

@urdarinda
Copy link
Author

@monkey92t please review

@monkey92t
Copy link
Collaborator

1、In the example, the demonstration is for tracing, but the actual implementation only includes metrics. Would you like to add tracing?

2、Based on the example and the code, the custom attributes seem to be global and cannot be changed based on execution commands or any conditions. In the go-redis hook, the ctx does not contain much useful information, making it difficult to modify custom variables through ctx.

3、

attrs := make([]attribute.KeyValue, 0, len(mh.attrs)+2)
attrs = append(attrs, mh.attrsFunc(ctx)...)
attrs = append(attrs, mh.attrs...)
attrs = append(attrs, attribute.String("type", "command"))
attrs = append(attrs, statusAttr(err))

This is a Go-related issue. Obviously, a slice expansion will occur here, and we should avoid it.

@urdarinda
Copy link
Author

urdarinda commented Mar 7, 2025

@monkey92t

1、In the example, the demonstration is for tracing, but the actual implementation only includes metrics. Would you like to add tracing?

Added for tracing, updated test as well

2、Based on the example and the code, the custom attributes seem to be global and cannot be changed based on execution commands or any conditions. In the go-redis hook, the ctx does not contain much useful information, making it difficult to modify custom variables through ctx.

This can be actually to augment the metrics/traces when used with other frameworks. I can add things like rpc.method etc to attribute the calling API to redis calls and create dashboards on top of that. Updated the example demonstrating that ( very crude)

3、

attrs := make([]attribute.KeyValue, 0, len(mh.attrs)+2)
attrs = append(attrs, mh.attrsFunc(ctx)...)
attrs = append(attrs, mh.attrs...)
attrs = append(attrs, attribute.String("type", "command"))
attrs = append(attrs, statusAttr(err))

This is a Go-related issue. Obviously, a slice expansion will occur here, and we should avoid it.

How do you suggest we do it? I believe this is the only way to expand a slice into individual values for now

@monkey92t
Copy link
Collaborator

For custom parameters, should we customize the parameters based on the form of a command or a key?

Regarding the Go slice issue, we should first calculate the length of the slice and then allocate memory. In the PR, only elements were appended to the slice without recalculating the slice length and allocating memory.

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.

4 participants