Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

Veneur-emit time #222

Merged
14 commits merged into from
Aug 18, 2017
Merged

Veneur-emit time #222

14 commits merged into from
Aug 18, 2017

Conversation

redsn0w422
Copy link
Contributor

@redsn0w422 redsn0w422 commented Aug 15, 2017

Summary

This PR adds a -command flag to veneur-emit. If you pass a command to -command, veneur-emit will:

  1. Skip/ignore any metrics by flag;
  2. Execute the command passed in;
  3. Report the runtime of that executable as a Timing metric.

Motivation

A lot of use cases of veneur-emit involve a shell script, where the user would get the time, run some program, get the time again, calculate the difference, and then send the elapsed time as a Timing metric via veneur-emit. This PR makes it easier to do so.

Test plan

I added tests for timeCommand.

Rollout/monitoring/revert plan

Puppet!

r? @cory-stripe

TODO:

  • Do we care about the exit status of the executable?
    We do - veneur-emit will grab the exit status and put it on the Timing metric as a tag: exit_status:0, for example.

  • cmd.Start() & cmd.Wait() OR cmd.Run()?
    cmd.Run() gets the job done, for now.

  • Do we want to have a timeout on the executable?
    Too many edge cases for a timeout, so if the user wishes, they can add a timeout to the command passed to veneur-emit.

  • Do we want the user to be able to send other metrics as well, or should we skip that entirely?
    Skip metrics. Calling veneur-emit is trivial, and if they want to send other metrics, they can just call it repeatedly.

  • Support sending the Timing via SSF?
    SSF does not (yet) support Timing metrics.

@stripe-ci
Copy link

Gerald Rule: Copy Observability on Veneur and Unilog pull requests

cc @stripe/observability
cc @stripe/observability-stripe

@cory-stripe
Copy link
Contributor

Some notes:

  • Please ask @aditya-stripe and @asf-stripe and @krisreeves-stripe to see if they have suggestions on making sure we don't reinvent shell handling.
  • We should return the exit status of the command as our exit status so we don't "hide" failures.
  • We should definitely support SSF
  • Timeout is pretty cool, but maybe worth a separate PR
  • I think we can skip other metrics for now, they can call this command again if they want them.

@ghost ghost force-pushed the yasha-veneur-time branch 3 times, most recently from af9837c to 90e77d8 Compare August 16, 2017 00:09
@ghost ghost force-pushed the yasha-veneur-time branch from 90e77d8 to 5ff4d93 Compare August 16, 2017 17:51
@krisreeves-stripe
Copy link
Contributor

krisreeves-stripe commented Aug 16, 2017

Summary from my conversation with @yasha-stripe:

This is a bit of an awkward usage of --. A more normal (to my eyes, which probably doesn't agree with Go convention anyway) invocation would look like veneur-emit -shellCommand foo -- --foo-switch (or, in the case of subcommands, veneur-emit subcommand -- --subcommand-switch. However, I think it's fine as-is, particularly if this is not an easy pattern to implement. It's much nicer to be able to entirely avoid shell quoting hassles than the alternatives I can think of. I'm not a total shell beast, so maybe there's a pattern I'm unaware of?

I'm +1

Copy link
Contributor

@asf-stripe asf-stripe left a comment

Choose a reason for hiding this comment

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

Two comments, the rest looks great (:

elapsed := time.Since(start)
exitTag := fmt.Sprintf("exit_status:%d", exitStatus)
tags = append(tags, exitTag)
logrus.Debugf("'%s' took %s", command, elapsed)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use %q here instead of '%s' for an auto-quoted string (:

logrus.WithError(err).Fatal("Error!")
}

command := strings.Join(flag.Args(), " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed, let's cut out the middle-sh and execute the command directly (:

@ghost ghost force-pushed the yasha-veneur-time branch from bb8dfa8 to 77cc117 Compare August 18, 2017 02:16
Copy link
Contributor

@asf-stripe asf-stripe left a comment

Choose a reason for hiding this comment

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

😍

mode = flag.String("mode", "metric", "Mode for veneur-emit. Must be one of: 'metric', 'event', 'sc'.")
debug = flag.Bool("debug", false, "Turns on debug messages.")
command = flag.String("command", "", "Command to time. This will exec 'command', time it, and emit a timer metric.")
shellCommand = flag.Bool("shellCommand", false, "Turns on timeCommand mode. TODO")
Copy link
Contributor

Choose a reason for hiding this comment

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

What's TODO here?

Copy link

Choose a reason for hiding this comment

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

Thanks! I forgot to write the rest of the documentation :)

@cory-stripe
Copy link
Contributor

👍

@ghost ghost added the approved label Aug 18, 2017
@ghost ghost merged commit f24c4e3 into master Aug 18, 2017
@ghost ghost deleted the yasha-veneur-time branch August 18, 2017 15:22
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants