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.

Sign up for GitHub

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

Jump to bottom

ServiceCheck metric does not respect configured prefix #94

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

ServiceCheck metric does not respect configured prefix #94

wants to merge 2 commits into from

Conversation

rpopovych
Copy link

Fixing #91

@rpopovych rpopovych changed the title ServiceCheck and StartTImer metrics do not respect configured prefix ServiceCheck metrics do not respect configured prefix Oct 17, 2019
@rpopovych rpopovych changed the title ServiceCheck metrics do not respect configured prefix ServiceCheck metric does not respect configured prefix Oct 17, 2019
lucaspimentel
lucaspimentel approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@lucaspimentel lucaspimentel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks

@arbll
Copy link
Member

arbll commented Oct 24, 2019

Adding a note here that this change would be breaking and we'll need to bump the major version if it gets included
cc @julien-lebot @ogaca-dd

@ogaca-dd
Copy link
Contributor

@rpopovych: Thank you for the PR.

Unfortunately, our other DogStatsD clients do not use prefix for Service Check and we would like to keep a consistent behavior across our DogStatsD clients.

I will track this request in our internal backlog to make a change in all our clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucaspimentel lucaspimentel lucaspimentel approved these changes

@julien-lebot julien-lebot Awaiting requested review from julien-lebot

@zippolyte zippolyte Awaiting requested review from zippolyte

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rpopovych @arbll @ogaca-dd @lucaspimentel