Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

fix!: always assign a trace ID to each request#1033

Merged
kjin merged 2 commits intogoogleapis:masterfrom
kjin:correlation
Jun 4, 2019
Merged

fix!: always assign a trace ID to each request#1033
kjin merged 2 commits intogoogleapis:masterfrom
kjin:correlation

Conversation

@kjin
Copy link
Copy Markdown
Contributor

@kjin kjin commented May 22, 2019

Fixes #1025

We already store untraced requests (via a single object representing ALL untraced requests) in CLS, this changes it so that these objects also have a trace ID (and span ID, though not sure if it is useful).

@kjin kjin changed the title fix: always assign a trace ID to each request [wip] fix: always assign a trace ID to each request May 22, 2019
@kjin
Copy link
Copy Markdown
Contributor Author

kjin commented May 22, 2019

Actually, marking this as WIP (don't review) while I figure out what to do when we call Tracer#createChildSpan.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 22, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2019

Codecov Report

Merging #1033 into master will decrease coverage by 0.02%.
The diff coverage is 92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1033      +/-   ##
==========================================
- Coverage   94.87%   94.85%   -0.03%     
==========================================
  Files          96       96              
  Lines        6288     6311      +23     
  Branches      494      496       +2     
==========================================
+ Hits         5966     5986      +20     
- Misses        165      166       +1     
- Partials      157      159       +2
Impacted Files Coverage Δ
test/test-index.ts 100% <100%> (ø) ⬆️
test/test-default-ignore-ah-health.ts 100% <100%> (ø) ⬆️
test/test-trace-api-none-cls.ts 100% <100%> (ø) ⬆️
test/test-cls.ts 97.59% <100%> (ø) ⬆️
src/constants.ts 100% <100%> (ø) ⬆️
test/test-trace-api.ts 100% <100%> (ø) ⬆️
src/trace-api.ts 89.25% <80%> (-1.43%) ⬇️
src/cls.ts 94.64% <80%> (-1.79%) ⬇️
src/span-data.ts 89.33% <90.9%> (+1.27%) ⬆️

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 23a990a...3d50315. Read the comment docs.

@kjin kjin changed the title [wip] fix: always assign a trace ID to each request fix: always assign a trace ID to each request May 22, 2019
@kjin kjin requested a review from a team May 23, 2019 00:20
Comment thread src/span-data.ts Outdated
* created because the Trace Agent was disabled.
*/
export const UNTRACED_CHILD_SPAN = createPhantomSpanData(SpanType.UNTRACED);
export const DISABLED_CHILD_SPAN = createPhantomSpanData(SpanType.UNTRACED);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to sound nit-picky...

Is there a reason why the concept was renamed on the left hand side but left the same on the right hand side?

Based on your decision to rename the export, it seems that you prefer external users to conceptually think about this concept as disabled. If that is the right conceptualization, why is not the right conceptualization for the internals in this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable formerly named UNTRACED_CHILD_SPAN is now only used when the Trace Agent is disabled which is why the LHS changed (where as before it is used for a disabled agent or for unsampled traces). As for the RHS, there continues to be multiple reasons for a span object to represent "untraced" work -- either unsampled, or disabled.

I could get behind the idea that we could just introduce UNSAMPLED and DISABLED in place of UNTRACED, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the rename. I won't block this PR, but conceptual clarity is important.

Comment thread src/span-data.ts Outdated
* created because the Trace Agent was disabled.
*/
export const UNTRACED_CHILD_SPAN = createPhantomSpanData(SpanType.UNTRACED);
export const DISABLED_CHILD_SPAN = createPhantomSpanData(SpanType.UNTRACED);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the rename. I won't block this PR, but conceptual clarity is important.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 4, 2019
@kjin kjin force-pushed the correlation branch 4 times, most recently from 2023925 to bedc655 Compare June 4, 2019 21:48
@kjin kjin changed the title fix: always assign a trace ID to each request fix!: always assign a trace ID to each request Jun 4, 2019
@kjin kjin merged commit 6b427ab into googleapis:master Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getCurrentContextId() returns null when not sampling

4 participants