Skip to content

V3#22

Merged
achille-roussel merged 29 commits intomasterfrom
v3
Feb 23, 2017
Merged

V3#22
achille-roussel merged 29 commits intomasterfrom
v3

Conversation

@achille-roussel
Copy link
Copy Markdown
Contributor

Hey guys, I'd like to introduce some breaking changes here, mainly to make the code easier to use, but also to simplify the design so the package is easier to test.

Changes

  • The stats.Engine type is now much simpler, instead of being a metrics aggregator with its own goroutine it's just carrying the context to produce metrics (namespace + tags + handlers).

  • Functions like httpstats.NewHandler where receiving a stats.Engine as first argument, which in practice was always nil because using the default engine is good enough. They've been refactored like this:

// use the default stats engine
func NewHandler(h http.Handler) http.Handler { ... }

// specify the stats engine
func NewHandlerWith(e *stats.Engine, h http.Handler) http.Handler { ... }
  • stats.Engine can now be cloned to inherit the tags that were set on it and add new ones, this was a missing feature that Prateek pointed out to me, for example:
// Same as the default engine but all metrics will have the hello:world tag
e := stats.DefaultEngine.Clone(stats.Tag{"hello", "world"})
  • The new design allows for zero memory allocations when publishing stats, thanks to a simpler architecture and the use of memory pools for the two objects that need dynamic memory allocations (stats.Metric and the buffer where it's serialized on the datadog client).

  • Thanks to the zero-allocation we're seeing great perf improvements, it takes ~600ns to publish a metric to the datadog client.

  • The buffer size of the datadog client has been adjusted so it doesn't overflow the agent, despite what they say that 8KB buffer size was added just recently, historically the agent has used a 1KB buffer.

  • I got rid of the in-process aggregation because the heavy use of tags greatly reduces the opportunities for merging metrics. This can be reintroduced later on within the datadog client.

  • Also a bunch more stuff like fixing docs, better name consistency, cleaner implementations, better defaults, more tests...

Upgrade

In most cases the work required to move to this new version is:

  1. Rewrite
handler = httpstats.NewHandler(nil, handler)

to

handler = httpstats.NewHandler(handler)
  1. Rewrite how the datadog client is instantiated, from
dd := datadog.NewClient(datadog.ClientConfig{
    Address: address,
})
defer dd.Close()

to

stats.Register(datadog.NewClient(address))
defer stats.Flush()

Please take a look and let me know what your thoughts are, also if you have any ideas to contribute this is the time!

@yields
Copy link
Copy Markdown
Contributor

yields commented Feb 23, 2017

// Same as the default engine but all metrics will have the hello:world tag
e := stats.DefaultEngine.Clone(stats.Tag{"hello", "world"})

What do you think about .WithTags() instead?


Thanks to the zero-allocation we're seeing great perf improvements, it takes ~600ns to publish a metric to the datadog client.

💥


stats.Register(datadog.NewClient(address))
defer stats.Flush()

That's wayyyy nicer :)

@achille-roussel
Copy link
Copy Markdown
Contributor Author

I'm good with WithTags, are you OK with renaming the Clone methods on metrics as well? (I'd like to keep the API consistent across types).

@yields
Copy link
Copy Markdown
Contributor

yields commented Feb 23, 2017

I'm good with WithTags, are you OK with renaming the Clone methods on metrics as well? (I'd like to keep the API consistent across types).

sgtm!

Copy link
Copy Markdown
Contributor

@tysonmote tysonmote left a comment

Choose a reason for hiding this comment

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

I didn't do a full deep dive on all the changes because it's such a large change, but on a cursory read-through this looks great!

Comment thread README.md Outdated
// Creates a new datadog client reporting the state of the default stats
// engine to localhost:8125.
dd := datadog.NewDefaultClient()
// Creates a new datadog client publishing emtrics to localhost:8125
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor typo: emtrics

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nice catch

Copy link
Copy Markdown
Contributor

@f2prateek f2prateek left a comment

Choose a reason for hiding this comment

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

Overall looks good!

@tysonmote
Copy link
Copy Markdown
Contributor

Ping me when kit is updated and I can try it out on a few warehouse services.

@achille-roussel
Copy link
Copy Markdown
Contributor Author

Thanks for the review guys!

@achille-roussel achille-roussel merged commit cfa6558 into master Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants