feat: Don't set ATTR_SERVICE_NAME in fastify instrumentation #70

bcomnes · 2025-06-24T22:01:55Z

This PR is adjacent to #69

In trying to understand how to use this otel instrument, I tried setting the SemanticResourceAttributes.SERVICE_NAME in the resource field of the NodeSDK instance in my otel setup. I was surprised to see that the fastify instrument wasn't inheriting this the way the other instruments were. I see that I can set this for both by exporting a OTEL_SERVICE_NAME, but if I set it in the NodeSDK constructor, I also have to pass a name to serverName in the fastify instrument, which is inconsistent with the other instruments.

According to the otel spec:

It seems like this is something that should be set on the resource, in the accompanying SDK and not something instruments should be concerned with.

For example, in the http instrument, ATTR_SERVICE_NAME is never set

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-http/src/utils.ts#L469-L481

Same with GRPC:

https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts#L456

Also the way this is implemented is that if you omit serverName and don't have OTEL_SERVICE_NAME exported, but pass in a service name to NodeSDK, your fastify spans will get tagged with a service name of fastify as the fallback for serverName rather than your service name, which definitely doesn't seem correct.

This would either be a bug fix or a breaking change, but given how long its been around it should probably be breaking.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

metcoder95

Can you add a tests that covers the NodeSDK path?

Eomm

I would like to understand more the issue here because it seems already handled to me

README.md

bcomnes · 2025-06-26T18:30:32Z

Can you add a tests that covers the NodeSDK path?

Yes, good idea. Will add something in.

chiawendt · 2025-08-12T17:04:12Z

I added a NodeSDK test in #86

chiawendt · 2025-09-05T07:48:39Z

Hi @bcomnes thanks you for your effort, can you rebase this PR? This bug is causing our spans to have wrong service_name=fastify.

bcomnes · 2025-09-05T13:58:33Z

Yes I will today

bcomnes · 2025-09-05T23:30:15Z

There were some conflicts I tried to address. Hopefully the tests still pass.

# Conflicts: # index.js # test/index.test.js

bcomnes · 2025-09-06T00:12:52Z

Took one final pass to remove a few more servername references. Definitely give it a one more once over but lets land it while its still fresh.

bcomnes · 2025-09-12T00:36:12Z

@Eomm Did these changes address your request? Sorry for the timeline on this one.

Copilot

Pull Request Overview

Align Fastify instrumentation with OpenTelemetry spec by removing explicit service.name span attributes and the servername option; rely on Resource for service naming.

Remove all ATTR_SERVICE_NAME usage and the servername configuration from the instrumentation.
Update tests to stop asserting on service.name span attributes; adjust docs to direct users to set service name via Resource (OTEL_SERVICE_NAME or NodeSDK).
Clean up typings/tests to reflect removal of servername.

Reviewed Changes

Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
index.js	Removes servername handling and all service.name attribute injections to follow OTel Resource semantics.
test/index.test.js	Updates expectations to no longer include service.name attributes.
test/sdk.test.js	Removes service.name assertions from spans when using NodeSDK.
test/env-vars.test.js	Renames test and drops service.name attribute assertions.
test/api.test.js	Removes API assertion for the servername option/property.
types/index.test-d.ts	Drops servername from config type assertions.
types/env-vars.test-d.ts	Removes servername from type assertions.
README.md	Documents that service name comes from Resource and removes servername option examples/docs.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-19T02:55:02Z

index.js

  constructor (config) {
    super(PACKAGE_NAME, PACKAGE_VERSION, config)
-    this.servername = config?.servername ?? process.env.OTEL_SERVICE_NAME ?? 'fastify'
    this.logger = diag.createComponentLogger({ namespace: PACKAGE_NAME })


[nitpick] serverName/servername has been removed, but passing it now gets silently ignored. Please emit a deprecation warning if config.servername is provided to guide users to set service name via Resource (OTEL_SERVICE_NAME or NodeSDK resource). For example, after line 56: if (config && 'servername' in config) { this.logger.warn('FastifyOtelInstrumentation: "servername" is no longer supported; set service name via Resource (OTEL_SERVICE_NAME or NodeSDK resource).'); }

Suggested change

this.logger = diag.createComponentLogger({ namespace: PACKAGE_NAME })

this.logger = diag.createComponentLogger({ namespace: PACKAGE_NAME })

// Deprecation warning for serverName/servername

if (config && ('serverName' in config || 'servername' in config)) {

this.logger.warn(

'FastifyOtelInstrumentation: "serverName"/"servername" is no longer supported; set service name via Resource (OTEL_SERVICE_NAME or NodeSDK resource).'

)

}

Copilot · 2025-09-19T02:55:03Z

README.md

+
+// Service name should be set via environment variable:
+// export OTEL_SERVICE_NAME=my-server
+// or via NodeSDK resource configuration


[nitpick] Consider adding an explicit migration note here to make the breaking change clear for users: // Note: the 'serverName'/'servername' option has been removed; set the service name via OTEL_SERVICE_NAME or NodeSDK Resource.

Suggested change

// or via NodeSDK resource configuration

// or via NodeSDK resource configuration

// Note: The 'serverName'/'servername' option has been removed.

// Set the service name via the OTEL_SERVICE_NAME environment variable or NodeSDK Resource.

bcomnes · 2025-09-25T21:37:11Z

Ping @Eomm in case you might have time this week 😅

chiawendt · 2025-09-30T06:58:24Z

Hi @gurgunday, can you help review this one?

mcollina

lgtm

mcollina · 2025-10-06T05:39:03Z

@bcomnes it's time to ship

bcomnes · 2025-10-06T07:09:58Z

I need someone to merge for me :)

Eomm

Sorry - I'm late 🙌

bcomnes · 2025-10-06T16:38:15Z

No worries thank you for reviewing!

This was referenced Jun 24, 2025

Make README example less pseudo-y #69

Draft

Otel WIP hifiwi-fi/breadcrum.net#515

Closed

metcoder95 reviewed Jun 25, 2025

View reviewed changes

Eomm requested changes Jun 26, 2025

View reviewed changes

README.md Show resolved Hide resolved

chiawendt mentioned this pull request Aug 12, 2025

test: add a NodeSDK test #86

Merged

4 tasks

bcomnes force-pushed the remove-serverName branch from 646d9da to 37315f9 Compare September 5, 2025 23:29

bcomnes force-pushed the remove-serverName branch from 37315f9 to e11032f Compare September 6, 2025 00:07

Remove serverName

ee287f9

# Conflicts: # index.js # test/index.test.js

bcomnes force-pushed the remove-serverName branch from e11032f to ee287f9 Compare September 6, 2025 00:11

bcomnes requested review from Eomm and metcoder95 September 6, 2025 00:42

metcoder95 approved these changes Sep 7, 2025

View reviewed changes

Uzlopak requested a review from Copilot September 19, 2025 02:50

Copilot AI reviewed Sep 19, 2025

View reviewed changes

Merge branch 'main' into remove-serverName

3091780

metcoder95 changed the title ~~Don't set ATTR_SERVICE_NAME in fastify instrumentation~~ feat: Don't set ATTR_SERVICE_NAME in fastify instrumentation Oct 2, 2025

mcollina approved these changes Oct 6, 2025

View reviewed changes

Eomm approved these changes Oct 6, 2025

View reviewed changes

Merge branch 'main' into remove-serverName

11947d8

Eomm merged commit aff602a into fastify:main Oct 6, 2025
17 checks passed

-    this.logger = diag.createComponentLogger({ namespace: PACKAGE_NAME })
+    this.logger = diag.createComponentLogger({ namespace: PACKAGE_NAME })
+    // Deprecation warning for serverName/servername
+    if (config && ('serverName' in config || 'servername' in config)) {
+      this.logger.warn(
+        'FastifyOtelInstrumentation: "serverName"/"servername" is no longer supported; set service name via Resource (OTEL_SERVICE_NAME or NodeSDK resource).'
+      )
+    }

Uh oh!

feat: Don't set ATTR_SERVICE_NAME in fastify instrumentation #70

feat: Don't set ATTR_SERVICE_NAME in fastify instrumentation #70

Uh oh!

Conversation

bcomnes commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bcomnes commented Jun 26, 2025

Uh oh!

chiawendt commented Aug 12, 2025

Uh oh!

chiawendt commented Sep 5, 2025

Uh oh!

bcomnes commented Sep 5, 2025

Uh oh!

bcomnes commented Sep 5, 2025

Uh oh!

bcomnes commented Sep 6, 2025

Uh oh!

bcomnes commented Sep 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

bcomnes commented Sep 25, 2025

Uh oh!

chiawendt commented Sep 30, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 6, 2025

Uh oh!

bcomnes commented Oct 6, 2025

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

bcomnes commented Oct 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bcomnes commented Jun 24, 2025 •

edited

Loading