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

Feature request: configure underlying metric used by statsd.timer #563

Closed
zhammer opened this issue May 14, 2020 · 10 comments
Closed

Feature request: configure underlying metric used by statsd.timer #563

zhammer opened this issue May 14, 2020 · 10 comments
Assignees
@hush-hush
Labels
resource/dogstatsd stale Stale - Bot reminder

Comments

@zhammer
Copy link

zhammer commented May 14, 2020

Hey datadog team! I'm curious what y'all think of making it so that folks can configure the underlying metruc used by statsd.timer. I just got off a call with our org's datadog reps and it seems that using distribution instead of histogram metrics for timers would be better for our orgs use cases.

The changes needed seem fairly straightforward: the desired metric could be passed into DogStatsd -> TimedContextManagerDecorator and then used to configure the metric TiemContextManagerDecorator uses to emit the timer stats:

datadogpy/datadog/dogstatsd/context.py

Line 75 in 0cb3794

self.statsd.timing(self.metric, elapsed, self.tags, self.sample_rate)

Also on a related note, I'm a little confused that TimedContextManagerDecorator emits a self.statsd.timing rather than self.statsd.histogram as suggested by the docs
@zhammer
Copy link
Author

zhammer commented May 14, 2020

Happy to open a PR and QA on our end if this is something you're interested in!

@zhammer
Copy link
Author

zhammer commented May 14, 2020

doing a quick local qa of this here: seatgeek@87d3c86

@jbarciauskas
Copy link
Contributor

@zhammer I think the best approach would be just to create a new decorator that calls statsd.distribution, would that work?

@hush-hush hush-hush self-assigned this May 21, 2020
@hush-hush
Copy link
Member

Hi @zhammer,

I agree that the feature would be very useful.

First of, to answer your question about timing vs histogram, Dogstatsd is an extension of the statsd protocol (we added things like event, service_check, distribution, ...), we're trying to remain compatible with the base protocol. That way statsd app can send metrics to Datadog through the Agent. Timing, or ms, is a statsd metric type but in the Agent it's aggregated and interpretated just like an histogram (it happens here if you're interested). That's what we mean by TIMER from StatsD is a sub-set of HISTOGRAM in DogStatsD in the documentation.

For the implementation we're always happy to receive PR. I agree with @jbarciauskas that a new decorator might be better. Any reason/use case where you might want to switch an entire application from histogram to distribution instead of choosing per TIMER call ?

@github-actions
Copy link

Thanks for your contribution!

This issue has been automatically marked as stale because it has not had activity in the last 30 days. Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity. Thank you for participating in the Datadog open source community.

If you would like this issue to remain open:

  1. Verify that you can still reproduce the issue in the latest version of this project.

  2. Comment that the issue is still reproducible and include updated details requested in the issue template.

@github-actions github-actions bot added the stale Stale - Bot reminder label Jun 21, 2020
@jbarciauskas
Copy link
Contributor

Hi @zhammer - is this something you're still interested in?

@zhammer
Copy link
Author

zhammer commented Jun 25, 2020

Hey @jbarciauskas! This is definitely still something our team is interested in. I've been a bit busy with some other work and forgot to respond.

@hush-hush - that is a very helpful description, thank you :) also, cool to see where that is actually happening in the agent code.

Any reason/use case where you might want to switch an entire application from histogram to distribution instead of choosing per TIMER call ?

Choosing per TIMER call seems good to me. I realize that even to migrate from using histogram to distribution timers in our apps, we may want to migrate specific timers/metrics in phases, so one global toggle for distribution vs. histogram would be tough. The only pros of using a global toggle I can think of would be:

  1. the datadog statsd methods stay in sync with the standard python statsd methods: https://statsd.readthedocs.io/en/v3.3/timing.html#using-a-context-manager
  2. devs on a codebase don't have to be aware of the underlying timer metric. they just add a timer and we've configured all timers to use (or at least default to) distributions. that way there isn't accidental sprawl of histogram vs distribution timers in one codebase.

For the implementation we're always happy to receive PR.

Happy to work on a PR, but this work is a bit lower priority at the moment so it could be some time until I have a PR to share.

@dnlserrano
Copy link
Contributor

Also very much interested in this. 🤓 I agree a specific decorator could work just fine (e.g., with statsd.distributed(...)), but I empathise with the suggestion from @zhammer of extending the statsd.timer existing one with an option for the desired timer metric, especially given this:

devs on a codebase don't have to be aware of the underlying timer metric

@hush-hush
Copy link
Member

First step will be a new decorator and then we can work on a global switch.

Right now I'm quite busy on other project but will come back to this as soon as I can (I'll create a card in our backlog to keep track of this). If this is a urgent matter I would be more than happy to review a PR !

@dnlserrano dnlserrano mentioned this issue Jun 27, 2020
Merged
6 tasks
dnlserrano added a commit to dnlserrano/datadogpy that referenced this issue Jun 27, 2020
@dnlserrano
Add distribution context manager and decorator
3e0d3c7 
A decorator or context manager that will measure the distribution of a
function's/context's run time using custom metric DISTRIBUTION.

E.g. of usage follows:

    @statsd.distributed('user.query.time', sample_rate=0.5)
    def get_user(user_id):
        # Do what you need to ...
        pass

    # Is equivalent to ...
    with statsd.distributed('user.query.time', sample_rate=0.5):
        # Do what you need to ...
        pass

    # Is equivalent to ...
    start = time.time()
    try:
        get_user(user_id)
    finally:
        statsd.distribution('user.query.time', time.time() - start)

Closes DataDog#563
@dnlserrano
Copy link
Contributor

Opened PR #581.

dnlserrano added a commit to dnlserrano/datadogpy that referenced this issue Jul 21, 2020
@dnlserrano
Add distribution context manager and decorator
8cd8f3e 
A decorator or context manager that will measure the distribution of a
function's/context's run time using custom metric DISTRIBUTION.

E.g. of usage follows:

    @statsd.distributed('user.query.time', sample_rate=0.5)
    def get_user(user_id):
        # Do what you need to ...
        pass

    # Is equivalent to ...
    with statsd.distributed('user.query.time', sample_rate=0.5):
        # Do what you need to ...
        pass

    # Is equivalent to ...
    start = time.time()
    try:
        get_user(user_id)
    finally:
        statsd.distribution('user.query.time', time.time() - start)

Closes DataDog#563
dnlserrano added a commit to dnlserrano/datadogpy that referenced this issue Jul 21, 2020
@dnlserrano
Add distribution context manager and decorator
da80e82 
A decorator or context manager that will measure the distribution of a
function's/context's run time using custom metric DISTRIBUTION.

E.g. of usage follows:

    @statsd.distributed('user.query.time', sample_rate=0.5)
    def get_user(user_id):
        # Do what you need to ...
        pass

    # Is equivalent to ...
    with statsd.distributed('user.query.time', sample_rate=0.5):
        # Do what you need to ...
        pass

    # Is equivalent to ...
    start = time.time()
    try:
        get_user(user_id)
    finally:
        statsd.distribution('user.query.time', time.time() - start)

Closes DataDog#563
@therve therve closed this as completed Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hush-hush hush-hush

Labels
resource/dogstatsd stale Stale - Bot reminder
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@therve @jbarciauskas @hush-hush @dnlserrano @zhammer