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

feat(shim-opentracing): update logging based on new spec #2282

Merged
merged 5 commits into from
Jun 30, 2021

Conversation

vreynolds
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • logging updates:
    • use event name when available
    • map error attributes to semantic conventions
    • use provided timestamp
  • fix shim example:
    • runtime error on accessing context
    • missing service.name

* use event name when available
* map error attributes to semantic conventions
* use provided timestamp
* fix shim example: runtime error and missing service.name
const entries = Object.entries(attributes);
const errorEntry = entries.find(([key]) => key === 'error.object');
const error = errorEntry?.[1];
if (typeof error === "string") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

string is the only "exception" type that's possible here, since the override method takes SpanAttributes, meaning shim consumers can't log an attribute like error.object: new Error(), if they're using type safety

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, recordException can take an Error object, but the log function itself takes SpanAttributes which don't allow objects as attribute values:

override log(keyValuePairs: SpanAttributes, _timestamp?: number): this {

The issue here is that the shim restricts the types more than the OpenTracing lib: https://github.com/opentracing/opentracing-javascript/blob/111ea4f7939c8f8f538333330d72115e5b28bcce/src/span.ts#L143 where you can pass any as attribute values, including objects.

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #2282 (6bd55df) into main (b09ee6f) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2282      +/-   ##
==========================================
+ Coverage   92.74%   92.79%   +0.04%     
==========================================
  Files         145      145              
  Lines        5184     5215      +31     
  Branches     1060     1068       +8     
==========================================
+ Hits         4808     4839      +31     
  Misses        376      376              
Impacted Files Coverage Δ
...ackages/opentelemetry-shim-opentracing/src/shim.ts 94.25% <100.00%> (+1.24%) ⬆️

@vreynolds
Copy link
Contributor Author

Looks like new node 16 build is unhappy with something in the NPM cache... I'm seeing the same error across other PRs that merged main in. Re-run with no cache?

@dyladan
Copy link
Member

dyladan commented Jun 23, 2021

re-running with no cache only helps for the first run, then after that it's broken again. i was looking into it this morning. looks like we'll have to add a chown or chmod to the ci action or something


provider.addSpanProcessor(new SimpleSpanProcessor(getExporter(serviceName)));
// Initialize the OpenTelemetry APIs to use the NodeTracerProvider bindings
provider.register();

registerInstrumentations({
});
registerInstrumentations({});
Copy link
Member

Choose a reason for hiding this comment

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

this will no longer register any default instrumentations so it can be removed or some default instrumentations should be added here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! Removed the line, since server.js and client.js generate spans inline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants