Updated Datadog metrics #1156
Updated Datadog metrics #1156
Conversation
Typing add spanName and safeTags to tracing options
…type Use interface instead of type to allow for declaration merging
Prevent registerInternalServices deprecation warning
|
Could you link some docs from Datadog regarding the changes? |
|
I updated comment in the code, e.g. /* More info: https://docs.datadoghq.com/api/latest/metrics/#submit-metrics */, what documents do you refer to? |
… DISCONNECT and come back with lower "seq" value.
.gitignore
Outdated
| @@ -11,3 +11,4 @@ benchmark/results | |||
| .prettierrc.js | |||
| .prettierrc | |||
| logs/ | |||
| .history/ | |||
There was a problem hiding this comment.
I have history plugin so it does not hurt to include this
src/metrics/registry.js
Outdated
| @@ -127,7 +127,7 @@ class MetricRegistry { | |||
| register(opts) { | |||
| if (!isPlainObject(opts)) throw new Error("Wrong argument. Must be an Object."); | |||
|
|
|||
| if (!opts.type) throw new Error("The metric 'type' property is mandatory."); | |||
| if (opts.type === undefined) throw new Error("The metric 'type' property is mandatory."); | |||
There was a problem hiding this comment.
Why did you change it? type == null or type == "" is valid? Please revert it.
There was a problem hiding this comment.
This is because !opts.type = 0 is true as metrics changed to numbers.
src/metrics/reporters/datadog.js
Outdated
| headers: { | ||
| "Content-Type": "application/json" | ||
| } | ||
| return fetch(`${this.opts.baseUrl}${this.opts.apiVersion}${this.opts.path}`, { |
There was a problem hiding this comment.
If the modified reporter only support v2, we should lock the apiVersion in the URL.
There was a problem hiding this comment.
Not sure, up to you. Will lock it.
src/metrics/types/index.js
Outdated
| @@ -24,9 +24,29 @@ const Types = { | |||
| */ | |||
| function getByName(name) { | |||
| /* istanbul ignore next */ | |||
| if (!name) return null; | |||
| if (name === undefined) return null; | |||
There was a problem hiding this comment.
Same problem, why name == null is valid?
There was a problem hiding this comment.
Again, probably because of numbers? 0?
src/metrics/types/index.js
Outdated
|
|
||
| let n = Object.keys(Types).find(n => n.toLowerCase() == name.toLowerCase()); | ||
| let converted = "Base"; | ||
| switch (name) { |
There was a problem hiding this comment.
It's a common code, not just Datadog specific, don't change it, because it will affect all reporters.
| @@ -176,10 +176,11 @@ describe("Test Datadog Reporter class", () => { | |||
|
|
|||
| expect(reporter.generateDatadogSeries).toBeCalledTimes(1); | |||
| expect(fetch).toBeCalledTimes(1); | |||
| expect(fetch).toBeCalledWith("https://api.datadoghq.com/api/v1/series?api_key=12345", { | |||
| expect(fetch).toBeCalledWith("https://api.datadoghq.com/api/v1/series", { | |||
| @@ -212,14 +213,14 @@ describe("Test Datadog Reporter class", () => { | |||
| }); | |||
| reporter.init(registry); | |||
|
|
|||
| registry.register({ name: "os.datetime.utc", type: "gauge" }).set(123456); | |||
| registry.register({ name: "test.info", type: "info" }).set("Test Value"); | |||
| registry.register({ name: "os.datetime.utc", type: 3 }).set(123456); | |||
There was a problem hiding this comment.
The registry is common, don't change the type. You should make the Datadog specific conversion inside the Datadog reporter and not in the common classes.
There was a problem hiding this comment.
But this is the test for datadog reporter, why is this common? They use numbers now, not names.
Fix merge schemas
Improves StartedStoppedHandler definition to return Promise with void[]
fix for #1169: getPrototypeOf instead of __proto__
Improve lifecycle handler types
chore(types): add service list options & define getActionList type
|
Something is wrong with this PR, it shows as merged but it's not, and it shows a lot of non-relevant changes as well. |
📝 Description
🎯 Relevant issues
💎 Type of change
📜 Example code
🚦 How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
🏁 Checklist: