-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add optional, per-query parameters for providing a traceparent and tags #654
Conversation
src/_http/index.js
Outdated
var result = '' | ||
const entries = Object.entries(tags) | ||
const entriesLength = entries.length | ||
var index = 1 | ||
entries.forEach(([key, value]) => { | ||
result += key + '=' + value | ||
if (index++ < entriesLength) result += ',' | ||
}) | ||
return result |
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.
return Object.entries(tags).map(e => e.join('=')).join()
maybe we want to validate the key:value pairs
silently throw out bad ones
let invalidTag = (value => [undefined, "", null].includes(value))
return Object.entries(tags)
.filter(e => !e.some(invalidTag))
.map(e => e.join('=')).join()
Exception
let invalidTag = (value => [undefined, "", null].includes(value))
return Object.entries(tags)
.map(e => {
if (e.some(invalidTag)){
throw new Error(`Invalid tag: ${e}`)
}
return e.join('=')
}).join()
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.
Missing ',' from the final join. Seems like it should be:
return Object.entries(tags).map(e => e.join('=')).join(',')
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.
comma is the default, but yes better to be verbose
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.
return Object.entries(tags).map(e => e.join('=')).join(',')
Much cleaner, thanks.
maybe we want to validate the key:value pairs
silently throw out bad ones
Exception is preferred to silently discarding. The only reason we silently discard invalid traceparents is because the W3C spec defines it that way, but in general, we should try to consider customer requests immutable. That said, let me sync with product on a reasonable length of tags and update that in a separate PR.
return Object.entries(tags) | ||
.map(e => e.join('=')) | ||
.join(',') |
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.
trim the keys and values?
reject empty/blank keys?
example code to help clarify:
> x = {'': 'dog'}
{ '': 'dog' }
> ' cat '.trim()
'cat'
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.
Yeah, gonna add validation in a follow-up (including max key/value length, etc.).
…gs (#654) * Add optional, per-query parameters for traceparent and tags * Fix merge conflicts * Fix tests. More merge conflicts * Address PR feedback
* Add optional, per-query parameters for providing a traceparent and tags (#654) * Add optional, per-query parameters for traceparent and tags * Fix merge conflicts * Fix tests. More merge conflicts * Address PR feedback * Tag validation (#658) * Add validation on per-query tags * Bump version for beta * Fix tags type (#661) * Fix tags type * 4.8.0-beta.2 * Force "logbeta" tag in publishConfig (#662) * Set version to 4.8.0 * Version * Run tests serially because reasons * Run all node versions in parallel * npm install * run npm install with v6 * Remove publishConfig block * Changelog * Skip failing test; passes locally * Removing beta note * Remove client validation and update tests --------- Co-authored-by: Trevor Finn <[email protected]> Co-authored-by: Cleve Stuart <[email protected]> Co-authored-by: Henry Ball <[email protected]>
Notes
FE-2382
Adds new, optional headers to driver,
traceparent
andtags
, for providing a unique identifier per-query identifier, as well as KV-pair tags, to each submitted query. README was updated as well:traceparent
A W3C-compliant identifier for enabling distributed tracing across different
vendors. If not provided, one is automatically generated server-side and attached to the query. See
Trace Context spec for more details.
tags
Allows for associating user-provided tags with a query.
How to test
Test cases were added to assert the new fields. They can be executed via
yarn test
.