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: datadog format supported for statsd driver #270

Closed
wants to merge 1 commit into from

Conversation

cv65kr
Copy link
Contributor

@cv65kr cv65kr commented Sep 15, 2022

Reason for This PR

Right now tags are combined into metric name. Since Datadog supports statsd but required different format, in result tags are allowed.
roadrunner-server/roadrunner#1288

Description of Changes

Add possibility to use datadog format for statsd driver - https://docs.datadoghq.com/developers/dogstatsd/datagram_shell/?tab=metrics

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

@rustatian
Copy link
Collaborator

Hey @cv65kr 👋🏻
You don't need to use a sample from the ticket from 2021. The client already has the needed formats.
Also, please, add tests here: https://github.com/roadrunner-server/rr-e2e-tests/tree/master/plugins/temporal
Temporal docker-compose is here: https://github.com/roadrunner-server/rr-e2e-tests/blob/master/env/docker-compose-temporal.yaml

Feel free to ping me on our Discord server.

@rustatian
Copy link
Collaborator

The leading case is not to use a specific driver (e.g. DataDog), because the only difference is a tag format, so we don't need an additional dependency.

@rustatian rustatian self-requested a review September 15, 2022 14:09
@rustatian rustatian marked this pull request as draft September 15, 2022 14:09
@rustatian rustatian changed the title Datadog format supported for statsd driver feat: datadog format supported for statsd driver Sep 15, 2022
@rustatian rustatian added the C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. label Sep 15, 2022
@rustatian rustatian added this to the v1.6.x milestone Sep 15, 2022
@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 15, 2022

@rustatian I have feeling like this part will not working in other way. The thing is we are using temporal reporter which is based on tally. Tally not supporting tags in basic implementation and also in this used in temporal

https://github.com/temporalio/temporal/blob/master/common/metrics/tally/statsd/reporter.go#L70
https://github.com/uber-go/tally/blob/master/statsd/reporter.go#L76

@rustatian
Copy link
Collaborator

Yeah, you're right; We need to re-implement a temporal MetricsHandler (inject or extend this interface).

@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 21, 2022

Hi @rustatian
Sorry for long time for my response. I prepared PR in tally repo to avoid creation of custom reporter - uber-go/tally#183

After merge of that, I will update this PR 😄

@rustatian
Copy link
Collaborator

Nice job, @cv65kr

@rustatian rustatian modified the milestones: v1.6.x, v1.7.x Oct 3, 2022
@rustatian rustatian modified the milestones: v1.7.x, v2.x Oct 21, 2022
@rustatian rustatian removed this from the v2.x milestone Jan 13, 2023
@rustatian
Copy link
Collaborator

@cv65kr Hey 👋🏻
Looks like Uber has finally updated the statsd to version 5. Could you please have a look, maybe we can move forward with this PR?

@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 14, 2023

Hi @rustatian version of statsd was updated, but they still not merged changes for tags supporting - https://github.com/uber-go/tally/blob/master/statsd/reporter.go#L76

@rustatian
Copy link
Collaborator

Yeah @cv65kr, you're right... Let's wait another year 😄

@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 14, 2023

@rustatian next year let's do a party to celebrate the anniversary 🎂

@rustatian
Copy link
Collaborator

Sorry, but I think that Uber doesn't will to maintain their repo and merge/review PR's. Closing this PR as staled. If in the 2030 they look at the PR and review it, I would be happy to accept your new PR ❤️

@rustatian rustatian closed this Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants