Skip to content

feat(resources)!: do not read environment variables from window #5466

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

Merged
pichlermarc merged 3 commits into from
Feb 13, 2025
+11 −71
Merged

feat(resources)!: do not read environment variables from window #5466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0cbbe0
feat(resources)!: do not read environment variables from window
pichlermarc Feb 6, 2025
b100bd6
fixup! feat(resources)!: do not read environment variables from window
pichlermarc Feb 13, 2025
b1568eb
Merge branch 'main' into feat/migrate-from-getenv-resources
pichlermarc Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se
* Renames `IResource` interface to `Resource`
* Export function `resourceFromAttributes` to create a `Resource` from a `DetectedAttributes` object
* Only export types and functions. This aids in cross-version compatibility and makes it more easily extensible in the future.
* feat(resources)!: do not read environment variables from window in browsers [#5466](https://github.com/open-telemetry/opentelemetry-js/pull/5466) @pichlermarc
* (user-facing): all configuration previously possible via `window.OTEL_*` is now not supported anymore
* If you have been using the `envDetector` in browser environments, please migrate to manually creating a resource.
* Note: Node.js environment variable configuration continues to work as-is.

### :rocket: (Enhancement)

Expand Down
7 changes: 3 additions & 4 deletions packages/opentelemetry-resources/src/detectors/EnvDetector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@
*/

import { Attributes, diag } from '@opentelemetry/api';
import { getEnv } from '@opentelemetry/core';
import { SEMRESATTRS_SERVICE_NAME } from '@opentelemetry/semantic-conventions';
import { ResourceDetectionConfig } from '../config';
import { DetectedResource, ResourceDetector } from '../types';
import { getStringFromEnv } from '@opentelemetry/core';

/**
* EnvDetector can be used to detect the presence of and create a Resource
Expand Down Expand Up @@ -53,10 +53,9 @@ class EnvDetector implements ResourceDetector {
*/
detect(_config?: ResourceDetectionConfig): DetectedResource {
const attributes: Attributes = {};
const env = getEnv();

const rawAttributes = env.OTEL_RESOURCE_ATTRIBUTES;
const serviceName = env.OTEL_SERVICE_NAME;
const rawAttributes = getStringFromEnv('OTEL_RESOURCE_ATTRIBUTES');
const serviceName = getStringFromEnv('OTEL_SERVICE_NAME');

if (rawAttributes) {
try {
Expand Down
71 changes: 4 additions & 67 deletions packages/opentelemetry-resources/test/detectors/browser/EnvDetector.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,76 +14,13 @@
* limitations under the License.
*/

import { RAW_ENVIRONMENT } from '@opentelemetry/core';
import * as assert from 'assert';
import { envDetector } from '../../../src';
import { describeBrowser } from '../../util';
import {
assertEmptyResource,
assertWebEngineResource,
} from '../../util/resource-assertions';
import { assertEmptyResource } from '../../util/resource-assertions';

describeBrowser('envDetector() on web browser', () => {
describe('with valid env', () => {
before(() => {
(
globalThis as typeof globalThis & RAW_ENVIRONMENT
).OTEL_RESOURCE_ATTRIBUTES =
'webengine.name="chromium",webengine.version="99",webengine.description="Chromium",custom.key="custom%20value"';
});

after(() => {
delete (globalThis as typeof globalThis & RAW_ENVIRONMENT)
.OTEL_RESOURCE_ATTRIBUTES;
});

it('should return resource information from environment variable', async () => {
const resource = envDetector.detect();
assert.ok(resource.attributes);
assertWebEngineResource(resource, {
name: 'chromium',
version: '99',
description: 'Chromium',
});
assert.strictEqual(resource.attributes['custom.key'], 'custom value');
});
});

describe('with invalid env', () => {
Copy link
Contributor

@maryliag maryliag Feb 13, 2025

Choose a reason for hiding this comment

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

with the new method is no longer possible to have invalid values?
or doesn't matter because it returns the empty and you're already testing that below?

Copy link
Member Author

@pichlermarc pichlermarc Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

These are the browser tests that before this PR allowed for users to set window.OTEL_SERVICE_NAME. Now on the browser this will always return an empty resource, as the new functions always return undefined in the browser.

That means that the EnvDetector is now a no-op detector for Browsers, hence no need to test many inputs anymore. 🙂

const values = ['webengine.description="with spaces"'];

for (const value of values) {
describe(`value: '${value}'`, () => {
before(() => {
(
globalThis as typeof globalThis & RAW_ENVIRONMENT
).OTEL_RESOURCE_ATTRIBUTES = value;
});

after(() => {
delete (globalThis as typeof globalThis & RAW_ENVIRONMENT)
.OTEL_RESOURCE_ATTRIBUTES;
});

it('should return empty resource', async () => {
const resource = envDetector.detect();
assertEmptyResource(resource);
});
});
}
});

describe('with empty env', () => {
it('should return empty resource', async () => {
const resource = envDetector.detect();
assertEmptyResource(resource);
});
});

describe('with empty env', () => {
it('should return empty resource', async () => {
const resource = envDetector.detect();
assertEmptyResource(resource);
});
it('should return empty resource', async () => {
const resource = envDetector.detect();
assertEmptyResource(resource);
});
});