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

SDK Resource #846

Merged
merged 17 commits into from Mar 11, 2020
Merged

SDK Resource #846

merged 17 commits into from Mar 11, 2020

Conversation

@mwear
Copy link
Member

mwear commented Mar 9, 2020

Which problem is this PR solving?

  • This PR adds a resource to TracerProvider and MeterProvider. Fixes #840.

Short description of the changes

  • This work adds a telemetry SDK resource to TracerProvider and MeterProvider and makes it available on associated spans and instruments. It uses thetelemetry.sdk semantic conventions that are proposed as a replacement for the library conventions that previously represented the SDK resource (open-telemetry/opentelemetry-specification#494)
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 9, 2020

Codecov Report

Merging #846 into master will increase coverage by 2.17%.
The diff coverage is 98.79%.

@@            Coverage Diff             @@
##           master     #846      +/-   ##
==========================================
+ Coverage   92.63%   94.81%   +2.17%     
==========================================
  Files         251      252       +1     
  Lines       11057    11032      -25     
  Branches     1079     1063      -16     
==========================================
+ Hits        10243    10460     +217     
+ Misses        814      572     -242
Impacted Files Coverage Δ
.../opentelemetry-exporter-jaeger/test/jaeger.test.ts 100% <ø> (ø) ⬆️
...es/opentelemetry-exporter-collector/test/helper.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-metrics/src/types.ts 100% <ø> (ø) ⬆️
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts 100% <ø> (ø) ⬆️
.../opentelemetry-base/src/platform/node/constants.ts 0% <0%> (ø)
packages/opentelemetry-metrics/src/Meter.ts 96.96% <100%> (+0.09%) ⬆️
packages/opentelemetry-tracing/src/Span.ts 100% <100%> (ø) ⬆️
...telemetry-tracing/test/BasicTracerProvider.test.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-metrics/src/Metric.ts 96.61% <100%> (+0.24%) ⬆️
...s/opentelemetry-tracing/src/BasicTracerProvider.ts 100% <100%> (ø) ⬆️
... and 64 more
Copy link
Member

mayurkale22 left a comment

Added first pass comments, I am still trying to understand how to use in MeterProvider and TracerProvider.

packages/opentelemetry-exporter-jaeger/package.json Outdated Show resolved Hide resolved
/** The name of the telemetry library. */
NAME: 'library.name',
NAME: 'telemetry.sdk.name',

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Mar 9, 2020

Member

Previous attributes names were aligned to the specs (see: https://github.com/open-telemetry/opentelemetry-specification/blob/948304ad9ddf754ca8308cd51f54c1e8ced5c7d2/specification/data-resource-semantic-conventions.md#library). Could you please help me to point corresponding specs issue/PR?

This comment has been minimized.

Copy link
@mwear

mwear Mar 9, 2020

Author Member

The proposed changes are here: open-telemetry/opentelemetry-specification#494. Let me know if it's too early to adopt them.

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Mar 9, 2020

Member

Found specs issue: open-telemetry/opentelemetry-specification#484, I would have made this change in separate PR because specs PR is still not merged.

This comment has been minimized.

Copy link
@mwear

mwear Mar 9, 2020

Author Member

I can go back to what is currently spec'd. Would you prefer that for this PR?

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Mar 9, 2020

Member

I think it is fine for now, you already made significant efforts to change in multiple places I don't want you to change again. Also, specs PR looks in good shape (3 ppl already approved it).

@@ -16,3 +16,4 @@

export { Resource } from './Resource';
export * from './constants';
export * from './resource-assertions';

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Mar 9, 2020

Member

I think no need to export internal assertion util,

This comment has been minimized.

Copy link
@mwear

mwear Mar 9, 2020

Author Member

Both this and #846 (comment) are so that other packages can use the utilities to validate resources. Currently this is used in tests in opentelemetry-web, opentelemetry-node, opentelemetry-metrics, opentelemetry-tracing. Let me know if this should change.

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Mar 9, 2020

Member

I see, but I am not sure if this is the right approach. Would like to hear from others.

This comment has been minimized.

Copy link
@mwear

mwear Mar 9, 2020

Author Member

It's probably sufficient for me to verify that a resource exists on the proper telemetry objects, without having to assert that it conforms to a specific semantic convention. I can go that route if it'd be better. Just let me know.

This comment has been minimized.

Copy link
@dyladan

dyladan Mar 10, 2020

Contributor

why not add it to @opentelemetry/test-utils?

This comment has been minimized.

Copy link
@mwear

mwear Mar 10, 2020

Author Member

I think this will create a circular dependency that I'm not sure how to resolve. If the resource-assertions move to test-utils, they will have a dependency on opentelemetry-resources. opentelemetry-resources will want to use the resource-assertions, so it will depend on test-utils. Leading to the dependency cycle opentelemetry/resources -> opentelemetry/test-utils -> opentelemetry/resources.

@@ -14,16 +14,17 @@
* limitations under the License.
*/

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Mar 9, 2020

Member

Why this file moved to src/ from test/?

@@ -24,6 +25,7 @@ import { DEFAULT_CONFIG, MeterConfig } from './types';
*/
export class MeterProvider implements types.MeterProvider {
private readonly _meters: Map<string, Meter> = new Map();
readonly resource: Resource = Resource.createTelemetrySDKResource();
readonly logger: types.Logger;

constructor(private _config: MeterConfig = DEFAULT_CONFIG) {

This comment has been minimized.

Copy link
@mayurkale22

mayurkale22 Mar 9, 2020

Member

What do you think about making resource part of MeterConfig with default value either to Resource.empty() or Resource.createTelemetrySDKResource()? This way users (and independent developers) can customize the resource attributes.

This comment has been minimized.

Copy link
@mwear

mwear Mar 10, 2020

Author Member

I went with Resource.createTelemetrySDKResource() as the default. When we have resource auto detection, it will be updated to use that (e.g. Resource.detectAll() or similar).

packages/opentelemetry-metrics/src/MeterProvider.ts Outdated Show resolved Hide resolved
Copy link
Member

mayurkale22 left a comment

LGTM

packages/opentelemetry-metrics/src/Meter.ts Outdated Show resolved Hide resolved
@mwear mwear force-pushed the mwear:sdk_resource branch from 5a03ae3 to 4c9c86a Mar 11, 2020
Copy link
Member

OlivierAlbertini left a comment

👍

@mayurkale22

This comment has been minimized.

Copy link
Member

mayurkale22 commented Mar 11, 2020

@open-telemetry/javascript-approvers we need more reviews on this one!

@mayurkale22 mayurkale22 merged commit abd191b into open-telemetry:master Mar 11, 2020
7 checks passed
7 checks passed
ci/circleci: lint_&_docs Your tests passed on CircleCI!
Details
ci/circleci: node10 Your tests passed on CircleCI!
Details
ci/circleci: node12 Your tests passed on CircleCI!
Details
ci/circleci: node12-browsers Your tests passed on CircleCI!
Details
ci/circleci: node13 Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
cla/linuxfoundation mwear authorized
Details
mayurkale22 added a commit to mayurkale22/opentelemetry-node that referenced this pull request Mar 11, 2020
mayurkale22 added a commit to mayurkale22/opentelemetry-node that referenced this pull request Mar 12, 2020
mayurkale22 added a commit that referenced this pull request Mar 12, 2020
* export resource to exporters

* update Zipkin and Stackdriver exporter to use Resource

* chore: remove redundant dependency

* update Collector exporter to use Resource

* rebase with #846

* minor

* fix collector resource

* fix build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.