Chore: Batch reporting for Graphite #26
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
igor-dv
approved these changes
Feb 2, 2022
|
|
||
| function validate({ name, value, type }) { | ||
| if (value === undefined || value === null) throw new TypeError(`${name} is missing`); | ||
| // eslint-disable-next-line valid-typeof |
There was a problem hiding this comment.
what's wrong with this one ?
There was a problem hiding this comment.
this rule requires a comparison to a literal string, Since I'm comparing to a parameter it fails the lint. In this specific case I want to use the parameter (thats the point of this function)
| describe('constructor', () => { | ||
| it('should create socket with given host and default values', () => { | ||
| // eslint-disable-next-line no-new | ||
| new StatsdSocket({ |
There was a problem hiding this comment.
so you still want to use constructor for this function ?
There was a problem hiding this comment.
In the meantime yes - I don't want to confuse different standards in the repo. We will change it as a single PR all across the repository (its backward compatible so its not a must for version 1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add batch reporting to graphite reporter.
TL;DR: Extract statsd logic and re-use it in both graphite and datadog, thus gaining batch reporting from the current datadog implementation
Since DD and Graphite both use statsd to report metrics, there were two options here:
Option 2 was more viable at this point since there are slight differences in the actual reporting format between DataDog and Graphite. It will also give more flexibility to modify the reporters according to the specific target instead of trying to over generalize and pay the cost. Furthermore, with composition we still gain re-use of the major parts and allowing other reporters to re-use it as well without creating hard dependencies between them.
resolves #21