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

feat: Add custom dimensions for metrics #122

Closed
wants to merge 10 commits into from
Closed

feat: Add custom dimensions for metrics #122

wants to merge 10 commits into from

Conversation

askpt
Copy link
Member

@askpt askpt commented Dec 21, 2023

This PR

  • Adding custom dimensions for metrics

Related Issues

Fixes #118

Notes

  • Unfortunately, this feature has the same issue as the initial metrics. I cannot assert if the tags are added correctly. See feat: Add Metrics Hook #114 for more info.
  • Removed the otel library from the unit tests so it will use the same one that is provided in the main project.

How to test

On the yellow boxes you can see the custom metrics.
Screenshot 2023-12-22 at 13 16 40

@askpt askpt marked this pull request as ready for review December 22, 2023 13:13
@askpt askpt requested a review from a team as a code owner December 22, 2023 13:13
Comment on lines 30 to 34
/// <param name="customDimensions">The optional custom dimensions.</param>
public MetricsHook(MetricHookCustomDimensions customDimensions = null)
{
_customDimensionsTagList = customDimensions?.GetTagList() ?? Array.Empty<KeyValuePair<string, object>>();

Copy link
Member

@toddbaert toddbaert Dec 29, 2023

Choose a reason for hiding this comment

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

The flexibility of this approach is a bit limited. I think a functional approach might be better. Instead of allowing users to define k/v pairs, I would recommend you allow users to define a function that takes an FlagEvaluationDetails object as a param, and returns a tuple or k/v set that will be added. Then users can build whatever data they want and add it as metrics. This will be even more valuable when we add flag metadata to this SDK.

We do something similar in JS: https://github.com/open-feature/js-sdk-contrib/blob/main/libs/hooks/open-telemetry/README.md#custom-attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

@toddbaert I was looking into the Java implementation and they offer a Flag Metadata (that is not implemented on dotnet SDK yet) and uses a custom dimensions that have a predefined list. Might be interesting to keep the extra dictionary and when we have the flag metadata implemented at SDK level, we use the FlagEvaluationDetails metadata?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this comment for so long @askpt . I'm not sure I fully understand what you're proposing. Maybe you can clarify.

In any case, I think that once you implement the metadata here, the functional approach used in Java and JS should work.

@toddbaert
Copy link
Member

@askpt I'm guessing based on your commits and the build status, you're still working on this, so I'll mark it draft for now. Feel free to mark it ready whenever.

@toddbaert toddbaert marked this pull request as draft January 12, 2024 20:39
@askpt askpt closed this by deleting the head repository Jan 23, 2024
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.

[OTel Hook] Add custom-dimensions support to Metrics Hook
3 participants