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.

Sign up for GitHub

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

Jump to bottom

refactor(instr-connect): use exported strings for attributes #2154

Merged
merged 2 commits into from May 2, 2024
Merged

refactor(instr-connect): use exported strings for attributes #2154

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
134dedd
refactor(instr-connect): use exported strings for attributes
david-luna Apr 26, 2024
4bb098a
Merge branch 'main' into dluna/connect-semconv-strings
blumamir May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions plugins/node/opentelemetry-instrumentation-connect/README.md
View file
Open in desktop
Expand Up @@ -51,6 +51,16 @@ registerInstrumentations({

See [examples/connect](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/examples/connect) for a short example.

## Semantic Conventions

This package uses `@opentelemetry/semantic-conventions` version `1.22+`, which implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md)

Attributes collected:

| Attribute | Short Description |
| ------------ | ---------------------------------- |
| `http.route` | The matched route (path template). |

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it can be helpful to also enumerate the other, non-spec-compliant attributes that are emitted by this instrumentation, as a reference for users and for them to know what to expect when using this instrumentation.

There seems to be 2 custom attributes at the moment:

export enum AttributeNames {
  CONNECT_TYPE = 'connect.type',
  CONNECT_NAME = 'connect.name',
}

Do you think eventually we will need to either remove them or add them to the specs? Or eventually, each instrumentation is allowed to record extra attributes for which we also need some sort of documentation and stability guarantee?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think eventually we will need to either remove them or add them to the specs?

I'm not sure. I reached out to @joaopgrassi (semconv maintainer) to ask what the general rules are for instrumentations that emit non-semconv telemetry. It'll be brought up in the semconv SIG meeting today 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @pichlermarc and @joaopgrassi .

Any update on this issue?

I think it should not block the current PR. The non-semantic attributes can also be removed / added to docs in followup PRs once we get more guidelines.

Copy link
Member

Choose a reason for hiding this comment

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

True we can probably open this as a new issue to look into and follow-up on.

Copy link
Member

Choose a reason for hiding this comment

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

I've created #2170 for follow-up

## Useful links

- For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
Expand Down
2 changes: 1 addition & 1 deletion plugins/node/opentelemetry-instrumentation-connect/package.json
View file
Open in desktop
Expand Up @@ -57,7 +57,7 @@
"dependencies": {
"@opentelemetry/core": "^1.8.0",
"@opentelemetry/instrumentation": "^0.51.0",
"@opentelemetry/semantic-conventions": "^1.0.0",
"@opentelemetry/semantic-conventions": "^1.22.0",
"@types/connect": "3.4.36"
},
"homepage": "https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/opentelemetry-instrumentation-connect#readme"
Expand Down
4 changes: 2 additions & 2 deletions plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@ import {
InstrumentationNodeModuleDefinition,
isWrapped,
} from '@opentelemetry/instrumentation';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions';
import {
replaceCurrentStackRoute,
addNewStackLayer,
Expand Down Expand Up @@ -104,7 +104,7 @@ export class ConnectInstrumentation extends InstrumentationBase {
const spanName = `${connectTypeName} - ${connectName}`;
const options: SpanOptions = {
attributes: {
[SemanticAttributes.HTTP_ROUTE]: routeName.length > 0 ? routeName : '/',
[SEMATTRS_HTTP_ROUTE]: routeName.length > 0 ? routeName : '/',
[AttributeNames.CONNECT_TYPE]: connectType,
[AttributeNames.CONNECT_NAME]: connectName,
},
Expand Down
8 changes: 4 additions & 4 deletions plugins/node/opentelemetry-instrumentation-connect/test/instrumentation.test.ts
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ import * as assert from 'assert';

import { context, trace } from '@opentelemetry/api';
import { RPCType, setRPCMetadata, RPCMetadata } from '@opentelemetry/core';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { SEMATTRS_HTTP_ROUTE } from '@opentelemetry/semantic-conventions';
import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
import {
Expand Down Expand Up @@ -144,7 +144,7 @@ describe('connect', () => {
assert.deepStrictEqual(span.attributes, {
'connect.type': 'middleware',
'connect.name': ANONYMOUS_NAME,
[SemanticAttributes.HTTP_ROUTE]: '/',
[SEMATTRS_HTTP_ROUTE]: '/',
});
assert.strictEqual(span.name, 'middleware - anonymous');
});
Expand All @@ -163,7 +163,7 @@ describe('connect', () => {
assert.deepStrictEqual(span.attributes, {
'connect.type': 'middleware',
'connect.name': 'middleware1',
[SemanticAttributes.HTTP_ROUTE]: '/',
[SEMATTRS_HTTP_ROUTE]: '/',
});
assert.strictEqual(span.name, 'middleware - middleware1');
});
Expand All @@ -181,7 +181,7 @@ describe('connect', () => {
assert.deepStrictEqual(span.attributes, {
'connect.type': 'request_handler',
'connect.name': '/foo',
[SemanticAttributes.HTTP_ROUTE]: '/foo',
[SEMATTRS_HTTP_ROUTE]: '/foo',
});
assert.strictEqual(span.name, 'request handler - /foo');
});
Expand Down