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

chore: 4.0.0 release proposal #1043

Merged
merged 1 commit into from
Jun 5, 2019
Merged

chore: 4.0.0 release proposal #1043

merged 1 commit into from
Jun 5, 2019

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Jun 4, 2019

@kjin kjin requested a review from a team June 4, 2019 23:10
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 4, 2019
kjin referenced this pull request Jun 4, 2019
BREAKING CHANGE: This change modifies/removes APIs that assume a
particular format for trace context headers; in other words, any
place where the user would deal with a stringified trace context,
they would now deal with a TraceContext object instead. This affects
three APIs: `getResponseTraceContext` (input/output has changed from
string to TraceContext), `createRootSpan` (input RootSpanOptions
now accepts a TraceContext instead of a string in the traceContext
field), and `Span#getTraceContext` (output has changed from string
to TraceContext).
@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #1043 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1043   +/-   ##
=======================================
  Coverage   94.85%   94.85%           
=======================================
  Files          96       96           
  Lines        6311     6311           
  Branches      496      496           
=======================================
  Hits         5986     5986           
  Misses        166      166           
  Partials      159      159

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b427ab...136f275. Read the comment docs.

@kjin kjin merged commit 230e21e into googleapis:master Jun 5, 2019
@AdriVanHoudt
Copy link
Contributor

We are using hapi's server.inject and this is the change I made for this release and I wanted to make sure I did it right.

                         // Add tracing header for Google Cloud Traces
-                        const traceContext = TraceApi.getResponseTraceContext(request.headers[TraceApi.constants.TRACE_CONTEXT_HEADER_NAME]);
+                        const traceContext = TraceApi.propagation.extract((key) => request.headers[key]);
                         if (traceContext) {
-                            inject.headers[TraceApi.constants.TRACE_CONTEXT_HEADER_NAME] = traceContext;
+                            TraceApi.propagation.inject((key, value) => {
+
+                                inject.headers[key] = value;
+                            }, traceContext);
                         }

server.inject basically does a call to the current server but without the whole network stack.

Also I think this document needs to be updated to reflect the new api: https://github.com/googleapis/cloud-trace-nodejs/blob/master/doc/trace-api.md#for-incoming-requests

@kjin
Copy link
Contributor Author

kjin commented Jun 6, 2019

@AdriVanHoudt Yes, that should be correct. And thanks for pointing out the doc that needs to be changed, I'll go ahead and do that.

@AdriVanHoudt
Copy link
Contributor

Awesome, thanks for checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants