-
Notifications
You must be signed in to change notification settings - Fork 370
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
feat: add x-goog-api-client headers for retry metrics #1920
Conversation
src/gcs-resumable-upload.ts
Outdated
@@ -27,6 +27,8 @@ import {GoogleAuth, GoogleAuthOptions} from 'google-auth-library'; | |||
import {Readable, Writable} from 'stream'; | |||
import retry = require('async-retry'); | |||
import {RetryOptions, PreconditionOptions} from './storage'; | |||
import * as packageJson from '../package.json'; |
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.
I didn't catch this line during our pairing, but I believe this may break package.json#files
for consumers (via npm i @google-cloud/storage
) as build/package.json
wouldn't be included [npm docs].
We currently use this pattern for including package.json
(a little messy):
Lines 617 to 625 in cbe2590
let packageJson: ReturnType<JSON['parse']> = {}; | |
try { | |
// if requiring from 'build' (default) | |
packageJson = require('../../package.json'); | |
} catch (e) { | |
// if requiring directly from TypeScript context | |
packageJson = require('../package.json'); | |
} |
We have a few options:
- Perhaps we can add
build/package.json
topackage.json#files
. Drawback: some tooling may believebuild
in itself is a separate package (aspackage.json
) and may misbehave. - We can use the pattern found in
src/storage
. Drawback: messy-looking.
CC: @bcoe - as I'm working on an easy package.json
import proposal for Node.js.
test/gcs-resumable-upload.ts
Outdated
@@ -67,6 +67,8 @@ const RESUMABLE_INCOMPLETE_STATUS_CODE = 308; | |||
/** 256 KiB */ | |||
const CHUNK_SIZE_MULTIPLE = 2 ** 18; | |||
const queryPath = '/?userProject=user-project-id'; | |||
const X_GOOG_API_HEADER_REGEX = | |||
/^gl-node\/[0-9]+\.[0-9]+\.[-.\w]+ gccl\/[0-9]+\.[0-9]+\.[-.\w]+ gccl-invocation-id\/[\w]{8}-[\w]{4}-[\w]{4}-[\w]{4}-[\w]{12}$/; |
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.
The gl-node/...
and gccl/...
portions may not match for tagged semver preview/beta releases.
Here's a suggestion:
/^gl-node\/[0-9]+\.[0-9]+\.[-.\w]+ gccl\/[0-9]+\.[0-9]+\.[-.\w]+ gccl-invocation-id\/[\w]{8}-[\w]{4}-[\w]{4}-[\w]{4}-[\w]{12}$/; | |
/^gl-node\/(?<nodeVersion>[^W]+) gccl\/(?<gccl>[^W]+) gccl-invocation-id\/(?<gcclInvocationId>[^W]+)$/ |
Which will allow easy access to named groups for testing:
const match = X_GOOG_API_HEADER_REGEX.exec(...);
assert(match);
// match.groups.nodeVersion
// match.groups.gccl
// match.groups.gcclInvocationId
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.
^this suggestion is optional
@@ -541,7 +549,9 @@ export class Upload extends Writable { | |||
this.params | |||
), | |||
data: metadata, | |||
headers: {}, | |||
headers: { | |||
'x-goog-api-client': `gl-node/${process.versions.node} gccl/${packageJson.version} gccl-invocation-id/${this.currentInvocationId.uri}`, |
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.
Just to clarify-- I spoke with @frankyn and he mentioned that each request in a resumable upload should have its own invocation id (unless it is a retry of a request that failed). Is that reflective of what's happening here?
(Looks like it is from the test cases, but wanted to double check)
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.
Correct, that is what is happening here. The invocation ID is only changed after a successful response. If a failure case is hit the same ID will be used on the retry. Each request (create a URI, send a chunk, etc...) all utilize a new ID.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕