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: more advanced types for sync/async resource #2

Closed
wants to merge 42 commits into from

Conversation

dyladan
Copy link

@dyladan dyladan commented Aug 13, 2020

This allows the resource on the span to be sync in the export pipeline while allowing it to be async in the tracer and removes the need for casting.

I only did tracing for the example, but the same should be possible for metrics.

@@ -194,6 +190,13 @@ export class Span implements api.Span, ReadableSpan {
return this._ended;
}

get resource(): Resource {
Copy link
Author

Choose a reason for hiding this comment

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

If the promise is resolved (after _resolveResources completes), then this will act like a regular sync resource to satisfy the ReadableSpan interface.

Copy link
Owner

Choose a reason for hiding this comment

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

Imagine the following situation

  1. Resource is provided as promise
  2. Start span
  3. Something is checking for resource, then just started span receives a resource which is Resource.createTelemetrySDKResource()
  4. Resource that was provided as a promise has been just resolved
  5. Span ended - export span
  6. Span is being exported with wrong resource instead of the one provided from promise

Another situation

  1. Resource is provided as promise
  2. Start span
  3. Span ended - export span
  4. Span is being exported with wrong resource instead of the one provided from promise
  5. Resource that was provided as a promise has been just resolved - but previously created span has been already exported with wrong resource

Copy link
Author

Choose a reason for hiding this comment

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

Something is checking for resource

Checking for resource isn't even a part of the public API until we get a "Readable Span", which is not the same as a span. It might be easier if we just send a plain object instead of the actual Span with a getter?

Span is being exported with wrong resource instead of the one provided from promise

No, the export will still have the correct resource as the span will not be sent to the exporter until the resource is resolved, and getters don't cache. Try it if you don't believe me.

Another situation

Span is being exported with wrong resource instead of the one provided from promise

Why? The span isn't actually exported until the resource resolves.

Copy link
Author

Choose a reason for hiding this comment

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

?

Copy link
Owner

Choose a reason for hiding this comment

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

I wrote you a message on gitter 4 days ago, please read :)

Copy link
Author

Choose a reason for hiding this comment

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

D: ok thanks

Too many communication channels to keep track of

* feat: fast span and trace id generation

* chore: remove unnecessary buffer alloc

* chore: minimize calls to random

* chore: lint

* chore: check if generated id is invalid (all 0)

* chore: unsigned right shift required to guarantee function in range
import { TraceParams } from './types';

/**
* This class represents a span.
*/
export class Span implements api.Span, ReadableSpan {
Copy link
Owner

Choose a reason for hiding this comment

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

why did you remove ReadableSpan ?

Copy link
Author

Choose a reason for hiding this comment

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

It can be added back.

@@ -194,6 +190,13 @@ export class Span implements api.Span, ReadableSpan {
return this._ended;
}

get resource(): Resource {
Copy link
Owner

Choose a reason for hiding this comment

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

Imagine the following situation

  1. Resource is provided as promise
  2. Start span
  3. Something is checking for resource, then just started span receives a resource which is Resource.createTelemetrySDKResource()
  4. Resource that was provided as a promise has been just resolved
  5. Span ended - export span
  6. Span is being exported with wrong resource instead of the one provided from promise

Another situation

  1. Resource is provided as promise
  2. Start span
  3. Span ended - export span
  4. Span is being exported with wrong resource instead of the one provided from promise
  5. Resource that was provided as a promise has been just resolved - but previously created span has been already exported with wrong resource

renovate-bot and others added 21 commits August 14, 2020 11:02
…-telemetry#1336)

Co-authored-by: Mayur Kale <mayurkale@google.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
… UpDownSumObserver (open-telemetry#1428)

Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
* feat: graceful shutdown for tracing and metrics

* fix: wording in test case

* fix: typo

* fix meterprovider config to use bracket notation

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* fix meterprovider config to use bracket notation

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* fix: add callbacks to shutdown methods

* fix: merge conflict

* simplify meter shutdown code

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* fix: fix one-liner

* private function name style fix

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* fix: naming of private member variables

* fix: graceful shutdown now works in browser

* fix: window event listener will trigger once

* fix: modify global shutdown helper functions

* fix: remove callback from remove listener args

* fix: change global shutdown function names and simplify functionality

* fix: add rest of function refactoring and simplification

* fix: remove unintended code snippet

* fix: refactor naming of listener cleanup function and fix sandbox issue

* fix: make global shutdown cleanup local

* fix: change interval of MeterProvider collection to ensure it does not trigger through clock

* chore: removing _cleanupGlobalShutdownListeners

* fix: remove unnecesary trace provider member function

* Removing default span attributes (open-telemetry#1342)

* refactor(opentelemetry-tracing): removing default span attributes

Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com>

* refactor(opentelemetry-tracing): removing default span attributed from tracer object

Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com>

* refactor(opentelemetry-tracing): removing accidental add to package.json

Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com>

* refactor(opentelemetry-tracing): removing redundant test and fixing suggestions by Shawn and Daniel

Signed-off-by: Aravin Sivakumar <aravinarjunn@gmail.com>

* feat: add baggage support to the opentracing shim (open-telemetry#918)

Co-authored-by: Mayur Kale <mayurkale@google.com>

* Add nodejs sdk package (open-telemetry#1187)

Co-authored-by: Naseem <naseemkullah@gmail.com>
Co-authored-by: legendecas <legendecas@gmail.com>
Co-authored-by: Mark Wolff <mrw00010@gmail.com>
Co-authored-by: Matthew Wear <matthew.wear@gmail.com>

* feat: add OTEL_LOG_LEVEL env var (open-telemetry#974)

* Proto update to latest to support arrays and maps (open-telemetry#1339)

* chore: 0.10.0 release proposal (open-telemetry#1345)

* fix: add missing grpc-js index (open-telemetry#1358)

* chore: 0.10.1 release proposal (open-telemetry#1359)

* feat(api/context-base): change compile target to es5 (open-telemetry#1368)

* Feat: Make ID generator configurable (open-telemetry#1331)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* fix: require grpc-js instead of grpc in grpc-js example (open-telemetry#1364)

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

* chore(deps): update all non-major dependencies (open-telemetry#1371)

* chore: bump metapackage dependencies (open-telemetry#1383)

* chore: 0.10.2 proposal (open-telemetry#1382)

* fix: remove unnecesary trace provider member function

* refactor(metrics): distinguish different aggregator types (open-telemetry#1325)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* Propagate b3 parentspanid and debug flag (open-telemetry#1346)

* feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary (open-telemetry#1320)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* feat: Collector Metric Exporter for the Web (open-telemetry#1308)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* Fix issues in TypeScript getting started example code (open-telemetry#1374)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>

* chore: deploy canary releases (open-telemetry#1384)

* fix: protos pull

* fix: address marius' feedback

* chore: deleting removeAllListeners from prometheus, fixing tests, cleanu of events when using shutdown notifier

* fix: add documentation and cleanup code

* fix: remove async label from shutdown and cleanup test case

* fix: update controller collect to return promise

* fix: make downsides of disabling graceful shutdown more apparent

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Aravin <34178459+aravinsiva@users.noreply.github.com>
Co-authored-by: Ruben Vargas Palma <ruben.vp8510@gmail.com>
Co-authored-by: Mayur Kale <mayurkale@google.com>
Co-authored-by: Naseem <naseemkullah@gmail.com>
Co-authored-by: legendecas <legendecas@gmail.com>
Co-authored-by: Mark Wolff <mrw00010@gmail.com>
Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Naseem <naseem@transit.app>
Co-authored-by: Mark Wolff <mark.wolff@microsoft.com>
Co-authored-by: Cong Zou <32532612+EdZou@users.noreply.github.com>
Co-authored-by: Reginald McDonald <40721169+reggiemcdonald@users.noreply.github.com>
Co-authored-by: WhiteSource Renovate <bot@renovateapp.com>
Co-authored-by: srjames90 <srjames@lightstep.com>
Co-authored-by: David W <dwitt12345@gmail.com>
Co-authored-by: Mick Dekkers <mickdekkersnl@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
…n-telemetry#1408)

* feat: temporarily added EC2 resource detector

* feat: update EC2 Resource Detector to IDMSv2 and modified detect-resource

* chore: fix weird package dependency changing

* chore: add final line for packages

* chore: modify descriptive type for _fetchString options

* chore: remove unused variables

* chore: modified test name

* refactor: replace request header with constant variables

* chore: modify comment

* refactor: remove HTTP_HEADER constant

* refactor: make time out value constant

* chore: revise the naming to IMDSv2

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
…metry#1366)

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
obecny and others added 16 commits August 24, 2020 18:34
* chore: removing submodule protos from exporter-collector

* chore: adding submodule opentelemetry-proto to exporter collector

* chore: fixing submodule path

* chore: updating proto to version v0.4.0

* chore: splitting exporter collector into 3 packages - depending on transport layer, updated examples, fixed the metrics collector for proto, fixed bug for label

* chore: fixing bug with controller when shutting down

* chore: ignored files

* chore: fixing submodule links

* chore: lint fixes - seems like some latest updates forcing extend to be in new line

* chore: lint space

* chore: fixing test when waiting to load proto files
* feat: adding possibility of recording exception

* chore: copy

* chore: copy

* chore: linting

* chore: reviews

* chore: updating exception type

* chore: reviews

* chore: reviews

* chore: fixing test when waiting for files to be loaded
Co-authored-by: Mayur Kale <mayurkale@google.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
…try#1344)

Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Co-authored-by: Bradley Behnke <bradley_behnke@intuit.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
* refactor: make spanContext into class with isValid

* refactor: use class instantiation for spanContext

* refactor: moves test and fix tests

* fix: add check for isValid

* revert: the changes to spanContext

* refactor: move spancontext-utils to api package

* fix: lint and invalid psan id and invalid trace id

* fix: make isValid more robust

* refactor: rename isValid to isSpanContextValid

* refactor: update regex and checks

* fix: export isSpanContextValid from api.trace

* fix: run npm run lint:fix

* fix: run lint fix in api

* refactor: prevent function overhead

* fix: lint

* fix: remove unused import

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
@dyladan
Copy link
Author

dyladan commented Sep 2, 2020

Closed for open-telemetry#1484

@dyladan dyladan closed this Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.