-
Notifications
You must be signed in to change notification settings - Fork 98
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.
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
fix: treat instanceId metadata as a number #713
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question.
src/trace-writer.ts
Outdated
const labels: LabelObject = {}; | ||
this.defaultLabels = {}; | ||
// tslint:disable-next-line:no-any | ||
const labels: {[key: string]: any} = {}; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/trace-writer.ts
Outdated
this.defaultLabels = labels; | ||
// Coerce values to strings. | ||
for (const key of Object.keys(labels)) { | ||
this.defaultLabels[key] = `${labels[key]}`; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #713 +/- ##
==========================================
+ Coverage 90.77% 90.78% +<.01%
==========================================
Files 29 29
Lines 1485 1486 +1
Branches 293 293
==========================================
+ Hits 1348 1349 +1
Misses 57 57
Partials 80 80
Continue to review full report at Codecov.
|
src/trace-writer.ts
Outdated
@@ -145,14 +145,14 @@ export class TraceWriter extends common.Service { | |||
'node ' + pjson.name + ' v' + pjson.version; | |||
labels[TraceLabels.GCE_HOSTNAME] = hostname; | |||
if (instanceId) { | |||
labels[TraceLabels.GCE_INSTANCE_ID] = instanceId; | |||
labels[TraceLabels.GCE_INSTANCE_ID] = `${instanceId}`; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/trace-writer.ts
Outdated
this.defaultLabels = {}; | ||
addDefaultLabel( | ||
TraceLabels.AGENT_DATA, | ||
'node ' + pjson.name + ' v' + pjson.version); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Fixes #712
This PR accounts for a change in #680 in how trace labels are applied. By directly assigning fields in a spans' labels instead of using
addLabel
, we were assuming that all default labels were strings. This isn't the case, because GCP instance IDs are numbers.