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

tagging and gauge operations #229

Closed
wants to merge 18 commits into from
Closed

tagging and gauge operations #229

wants to merge 18 commits into from

Conversation

gandarez
Copy link

@gandarez gandarez commented Mar 6, 2020

  • add support for tagging
  • add support for gauge operations

Related to the issues 175 and 176.

@gandarez gandarez requested a review from a team as a code owner March 6, 2020 19:50
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/IStatsDPublisher.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/IStatsDPublisher.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/PublicAPI.Shipped.txt Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDMessage.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDMessage.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/BufferBasedStatsDPublisher.cs Outdated Show resolved Hide resolved
@martincostello
Copy link
Member

Thank you for you contribution @gandarez!

Tags are fairly self-explanatory, but can you explain a little more about what the gauge changes are for please?

@gandarez gandarez changed the title tagging and guage operations tagging and gauge operations Mar 9, 2020
@gandarez
Copy link
Author

@martincostello thanks!

Fo all the comments you've done I already fixed them locally. There are only two left I'd need your assistance. The first is about the Shipped.txt and the other is how to I start integration tests in my machine.

I followed the documentation for gauges and implemented the concept of being able to change the sign for a gauge.
https://github.com/statsd/statsd/blob/master/docs/metric_types.md#gauges

@gandarez
Copy link
Author

gandarez commented Mar 10, 2020

@martincostello please be adivised that I needed to add keyNameSanitize: false to avoid the backend to remove non-alpha like semicolon and equal. This kind of behaviour is not expected (and actually does not happen) on graphite.

https://graphite.readthedocs.io/en/latest/tags.html

src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/Buffered/StatsDUtf8Formatter.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/IStatsDPublisherExtensions.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/IStatsDPublisherExtensions.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/IStatsDPublisherExtensions.cs Outdated Show resolved Hide resolved
src/JustEat.StatsD/PublicAPI.Shipped.txt Show resolved Hide resolved
@@ -1,4 +1,5 @@
{
keyNameSanitize: false,
Copy link
Member

Choose a reason for hiding this comment

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

If this is needed to make tags work in general, we'd probably want this documented somewhere in the README.

Which reminds me, the README would also need to be updated to mention the tagging capabilities.

Copy link
Member

Choose a reason for hiding this comment

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

Some guidance in the need to use keyNameSanitize if using tags would still be useful in the README.

Copy link
Author

Choose a reason for hiding this comment

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

I think it should be always false since the tests cover tags and the StatsD server is mocked. In real life this flag does not exist, for instance when running on Graphite.

Copy link
Member

Choose a reason for hiding this comment

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

@martincostello martincostello added this to the Future milestone Mar 14, 2020
@martincostello martincostello modified the milestones: Future, v5.0.0 Mar 14, 2020
@martincostello martincostello linked an issue Mar 14, 2020 that may be closed by this pull request
@martincostello martincostello linked an issue Mar 14, 2020 that may be closed by this pull request
@gandarez
Copy link
Author

Any news here guys?

@jdiazmartin
Copy link

I'm looking forward to having a fine-grained metrics with tags. Is there an approximate date for version 5.0.0?

@martincostello
Copy link
Member

I'm looking forward to having a fine-grained metrics with tags. Is there an approximate date for version 5.0.0?

Sorry, there's no planned release date at this time. This is something we'll pick up in more detail when we have the capacity for it.

@stale

This comment has been minimized.

@stale stale bot added the wontfix label Jun 2, 2020
@stale

This comment has been minimized.

@stale stale bot added the wontfix label Jul 29, 2020
@gandarez
Copy link
Author

The PR will be clossed automatically soon.. no plans yet??

@martincostello martincostello mentioned this pull request Aug 26, 2020
@martincostello
Copy link
Member

Hi @gandarez - sorry for the lack of movement on this PR.

For tag support, we're going to take the change from #269 as it is implemented in a more extensible way for a range of different metrics providers.

Once that is merged, if you update this PR to just suggest changes to gauges, we can re-review this PR.

Thanks!

@stale

This comment has been minimized.

@stale stale bot added the wontfix label Dec 12, 2020
Base automatically changed from master to main February 17, 2021 12:46
@martincostello
Copy link
Member

#312 to add tagging support has been merged.

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 26, 2021
@stale
Copy link

stale bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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.

Counter should not have decrement method Gauge increment
3 participants