Skip to content
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

Merged
merged 4 commits into from
Sep 14, 2022

Conversation

trevor-fauna
Copy link
Contributor

Notes

FE-2382

Adds new, optional headers to driver, traceparent and tags, 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.

var data = client.query(q.Paginate(q.Collections()), {
  traceparent: "00-c91308c112be8448dd34dc6191567fa0-b7ad6b7169203331-01",
})
tags

Allows for associating user-provided tags with a query.

var data = client.query(q.Paginate(q.Collections()), {
  tags: { key1: "value1", key2: "value2" },
})

How to test

Test cases were added to assert the new fields. They can be executed via yarn test.

@trevor-fauna trevor-fauna changed the title V4 traceparent Add optional, per-query parameters for providing a traceparent and tags Sep 12, 2022
Comment on lines 157 to 165
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
Copy link
Member

@mwilde345 mwilde345 Sep 12, 2022

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() 

Copy link
Contributor

@faunaee faunaee Sep 12, 2022

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(',')

Copy link
Member

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

Copy link
Contributor Author

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.

@trevor-fauna trevor-fauna merged commit 28072fc into fauna_logs Sep 14, 2022
@trevor-fauna trevor-fauna deleted the v4-traceparent branch September 14, 2022 12:49
Comment on lines +157 to +159
return Object.entries(tags)
.map(e => e.join('='))
.join(',')
Copy link
Contributor

@cleve-fauna cleve-fauna Sep 14, 2022

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'

Copy link
Contributor Author

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.).

adambollen pushed a commit that referenced this pull request Feb 10, 2023
…gs (#654)

* Add optional, per-query parameters for traceparent and tags

* Fix merge conflicts

* Fix tests. More merge conflicts

* Address PR feedback
adambollen added a commit that referenced this pull request Feb 10, 2023
* 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]>
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