Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

feat(core): start tracer with span attributes #514

Merged

Conversation

hekike
Copy link
Contributor

@hekike hekike commented May 9, 2019

Default attributes for Tracer that will be added to every Span created by Tracer.
Useful for system level attributes like cluster information.

@codecov-io
Copy link

codecov-io commented May 9, 2019

Codecov Report

Merging #514 into master will decrease coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
- Coverage   95.03%   94.81%   -0.22%     
==========================================
  Files         146      147       +1     
  Lines        9660    10571     +911     
  Branches      802      898      +96     
==========================================
+ Hits         9180    10023     +843     
- Misses        480      548      +68
Impacted Files Coverage Δ
test/test-http2.ts 77.83% <0%> (-20.2%) ⬇️
src/zpages-frontend/page-handlers/templates-dir.ts 80% <0%> (-20%) ⬇️
src/http2.ts 80.64% <0%> (-8.65%) ⬇️
src/internal/cls.ts 91.66% <0%> (-8.34%) ⬇️
src/tags/propagation/text-format.ts 95.55% <0%> (-4.45%) ⬇️
test/test-tracecontext-format.ts 98.33% <0%> (-0.77%) ⬇️
src/tags/propagation/binary-serializer.ts 97.1% <0%> (-0.63%) ⬇️
src/internal/string-utils.ts 100% <0%> (ø) ⬆️
src/index.ts 100% <0%> (ø) ⬆️
src/http-stats.ts 100% <0%> (ø) ⬆️
... and 56 more

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 89f20bb...9679242. Read the comment docs.

@@ -32,6 +33,8 @@ export interface BufferConfig {

/** Defines tracer configuration parameters */
export interface TracerConfig {
/** A set of attributes each in the format [KEY]:[VALUE] */
attributes?: Attributes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe calling it spanAttributes? defaultSpanAttributes?

Copy link
Member

Choose a reason for hiding this comment

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

Another option is either constantAttributes or defaultAttributes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the change, would you mind to update CHANGELOG.md with this new feature?

@@ -124,6 +124,12 @@ export class CoreTracerBase implements types.TracerBase {
if (sampleDecision) {
const rootSpan =
new RootSpan(this, name, kind, traceId, parentSpanId, traceState);
// Add tracer attributes
if (this.config.attributes) {
for (const key of Object.keys(this.config.attributes)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to use forEach statement for better readability. WDYT?

@@ -32,6 +33,8 @@ export interface BufferConfig {

/** Defines tracer configuration parameters */
export interface TracerConfig {
/** A set of attributes each in the format [KEY]:[VALUE] */
attributes?: Attributes;
Copy link
Member

Choose a reason for hiding this comment

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

Another option is either constantAttributes or defaultAttributes.

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Added minor comments.

@hekike
Copy link
Contributor Author

hekike commented May 10, 2019

@mayurkale22 thanks for the review, I renamed attributes to defaultAttributes and refactored the code based on your comments.

@hekike
Copy link
Contributor Author

hekike commented May 10, 2019

Added to changelog.
(For OpenTelemetry it would be useful to have automatic changelog generation via https://www.conventionalcommits.org)

@mayurkale22
Copy link
Member

Added to changelog.
(For OpenTelemetry it would be useful to have automatic changelog generation via https://www.conventionalcommits.org)

Not familiar with this tool, but happy to explore more. Thanks for sharing the link.

@mayurkale22 mayurkale22 merged commit 8cc92a4 into census-instrumentation:master May 10, 2019
@hekike hekike deleted the feat/core-tracer-attributes branch May 10, 2019 18:28
@hekike
Copy link
Contributor Author

hekike commented May 10, 2019

The links is just the spec for the commit message format.
The tools around it are:
https://github.com/conventional-changelog/conventional-changelog

For non-monorepo:
https://github.com/Netflix/unleash

etc.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants