Skip to content

Commit

Permalink
Merge branch 'main' into domas-fix-parsing-relative-urls
Browse files Browse the repository at this point in the history
  • Loading branch information
dyladan authored Mar 17, 2023
2 parents f813b6c + 7e9ff24 commit 44d6a51
Show file tree
Hide file tree
Showing 29 changed files with 2,002 additions and 80 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,18 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :rocket: (Enhancement)

* feat(sdk-metrics): add exponential histogram accumulation / aggregator [#3505](https://github.com/open-telemetry/opentelemetry-js/pull/3505) @mwear

### :bug: (Bug Fix)

* fix(sdk-trace-web): make `parseUrl()` respect document.baseURI [#3670](https://github.com/open-telemetry/opentelemetry-js/pull/3670) @domasx2
* fix(resource): make properties for async resource resolution optional [#3677](https://github.com/open-telemetry/opentelemetry-js/pull/3677) @pichlermarc
* fix(resources): change fs/promises import to be node 12 compatible [#3681](https://github.com/open-telemetry/opentelemetry-js/pull/3681) @pichlermarc

### :books: (Refine Doc)

* doc(sdk): update NodeSDK example [#3684](https://github.com/open-telemetry/opentelemetry-js/pull/3684) @martinkuba

### :house: (Internal)

## 1.10.0
Expand All @@ -37,6 +43,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

* fix(core): added falsy check to make otel core work with browser where webpack config had process as false or null [#3613](https://github.com/open-telemetry/opentelemetry-js/issues/3613) @ravindra-dyte
* fix(instrumentation-http): include query params in http.target [#3646](https://github.com/open-telemetry/opentelemetry-js/pull/3646) @kobi-co
* fix(sdk-metrics): merge uncollected delta accumulations [#3667](https://github.com/open-telemetry/opentelemetry-js/pull/3667) @legendecas

### :books: (Refine Doc)

Expand Down
84 changes: 50 additions & 34 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,25 @@

We'd love your help!

- [Development Quick Start](#development-quick-start)
- [Pull Request Merge Guidelines](#pull-request-merge-guidelines)
- [General Merge Requirements](#general-merge-requirements)
- [Report a bug or requesting feature](#report-a-bug-or-requesting-feature)
- [How to contribute](#how-to-contribute)
- [Before you start](#before-you-start)
- [Conventional commit](#conventional-commit)
- [Changelog](#changelog)
- [Fork](#fork)
- [Development](#development)
- [Tools used](#tools-used)
- [Install dependencies](#install-dependencies)
- [Compile modules](#compile-modules)
- [Running tests](#running-tests)
- [Linting](#linting)
- [Generating docs](#generating-docs)
- [Adding a package](#adding-a-package)
- [Platform conditional exports](#platform-conditional-exports)

## Development Quick Start

To get the project started quickly, you can follow these steps. For more
Expand All @@ -15,6 +34,37 @@ npm run compile
npm test
```

## Pull Request Merge Guidelines

Most pull requests MAY be merged by an approver OR a maintainer provided they meet the following [General Merge Requirements](#general-merge-requirements).
All requirements are at the discretion of the maintainers.
Maintainers MAY merge pull requests which have not strictly met these requirements.
Maintainers MAY close, block, or put on hold pull requests even if they have strictly met these requirements.

It is generally expected that a maintainer ([@open-telemetry/javascript-maintainers](https://github.com/orgs/open-telemetry/teams/javascript-maintainers)) should review and merge major changes.
Some examples include, but are not limited to:

- API changes
- Breaking changes
- New modules
- Changes which affect runtime support
- New features which are not required by the specification

If a PR has not been interacted with by a reviewer within one week, please ping the approvers ([@open-telemetry/javascript-approvers](https://github.com/orgs/open-telemetry/teams/javascript-approvers)).

### General Merge Requirements

- No “changes requested” reviews by approvers, maintainers, technical committee members, or subject matter experts
- No unresolved conversations
- Approved by at least one maintainer OR by at least one approver who is not the approver merging the pull request
- A pull request for small (simple typo, URL, update docs, or grammatical fix) changes may be approved and merged by the same approver
- For plugins, exporters, and propagators approval of the original code module author, or a contributor who has done extensive work on the module, is preferred but not required
- New or changed functionality is tested by unit tests
- New or changed functionality is documented if appropriate
- Substantial changes should not be merged within 24 hours of opening in order to allow reviewers from all time zones to have a chance to review

If all of the above requirements are met and there are no unresolved discussions, a pull request may be merged by either a maintainer or an approver.

## Report a bug or requesting feature

Reporting bugs is an important contribution. Please make sure to include:
Expand Down Expand Up @@ -216,40 +266,6 @@ To add a new package, copy `packages/template` to your new package directory and

After adding the package, run `npm install` from the root of the project. This will update the `tsconfig.json` project references automatically and install all dependencies in your new package. For packages supporting browser, file `tsconfig.esm.json` needs to be manually updated to include reference to ES modules build.

### Guidelines for Pull Requests

- Typically we try to turn around reviews within one to two business days.
- It is generally expected that a maintainer ([@open-telemetry/javascript-maintainers](https://github.com/orgs/open-telemetry/teams/javascript-maintainers)) should review and merge every PR.
- If a change has met the requirements listed below, an approver may also merge the pull request.
- Most PRs should be merged in one to two weeks.
- If a PR is taking longer than 30 days, please ping the approvers ([@open-telemetry/javascript-approvers](https://github.com/orgs/open-telemetry/teams/javascript-approvers)) as it may have been lost
- Dependency upgrades and Security fixes: This PR is small and/or low-risk and can be merged with only maintainer reviews.
- If your patch is not getting reviewed or you need a specific person to review it, you can @username or @open-telemetry/javascript-approvers a reviewer asking for a review in the pull request.
- API changes, breaking changes, or large changes will be subject to more scrutiny and may require more reviewers. These PRs should only be merged by maintainers.
- Changes to existing plugins and exporters will typically require the approval of the original plugin/exporter author.

### General Merge Requirements

- All requirements are at the discretion of the maintainers.
- Maintainers may merge pull requests which have not strictly met these requirements.
- Maintainers may close, block, or put on hold pull requests even if they have strictly met these requirements.
- No “changes requested” reviews.
- No unresolved conversations.
- 3 approvals, including the approvals of at least 2 maintainers
- A pull request opened by an approver may be merged with only the 2 maintainer reviews.
- Small (simple typo, URL, update docs, or grammatical fix) or high-priority changes may be merged more quickly or with fewer reviewers at the discretion of the maintainers. This is typically indicated with the express label.
- For plugins, exporters, and propagators approval of the original code module author is preferred but not required.
- New or changed functionality is tested by unit tests.
- New or changed functionality is documented.
- Substantial changes should not be merged within 24 hours of opening in order to allow reviewers from all time zones to have a chance to review.

If all of the above requirements are met and there are no unresolved discussions, a pull request may be merged by either a maintainer or an approver.

### Generating CHANGELOG documentation

- Generate and export your [Github access token](https://docs.github.com/en/github/authenticating-to-github/creating-a-personal-access-token): `export GITHUB_AUTH=<your_token>`
- `npm run changelog` to generate CHANGELOG documentation in your terminal (see [RELEASING.md](RELEASING.md) for more details).

### Platform conditional exports

Universal packages are packages that can be used in both web browsers and
Expand Down
6 changes: 1 addition & 5 deletions experimental/packages/opentelemetry-sdk-node/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,7 @@ const sdk = new opentelemetry.NodeSDK({
// See the Configuration section below for additional configuration options
});

// You can optionally detect resources asynchronously from the environment.
// Detected resources are merged with the resources provided in the SDK configuration.
sdk.start().then(() => {
// Resources have been detected and SDK is started
});
sdk.start();

// You can also use the shutdown method to gracefully shut down the SDK before process shutdown
// or on some operating system signal.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
AggregationTemporality,
DataPoint,
DataPointType,
ExponentialHistogram,
Histogram,
MetricData,
ResourceMetrics,
Expand Down Expand Up @@ -96,7 +97,10 @@ export function toMetric(metricData: MetricData): IMetric {
}

function toSingularDataPoint(
dataPoint: DataPoint<number> | DataPoint<Histogram>,
dataPoint:
| DataPoint<number>
| DataPoint<Histogram>
| DataPoint<ExponentialHistogram>,
valueType: ValueType
) {
const out: INumberDataPoint = {
Expand Down
16 changes: 9 additions & 7 deletions packages/opentelemetry-resources/src/Resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,17 @@ import { IResource } from './IResource';
*/
export class Resource implements IResource {
static readonly EMPTY = new Resource({});
private _syncAttributes: ResourceAttributes;
private _asyncAttributesPromise: Promise<ResourceAttributes> | undefined;
private _syncAttributes?: ResourceAttributes;
private _asyncAttributesPromise?: Promise<ResourceAttributes>;
private _attributes?: ResourceAttributes;

/**
* Check if async attributes have resolved. This is useful to avoid awaiting
* waitForAsyncAttributes (which will introduce asynchronous behavior) when not necessary.
*
* @returns true if the resource "attributes" property is not yet settled to its final value
*/
public asyncAttributesPending: boolean;
public asyncAttributesPending?: boolean;

/**
* Returns an empty Resource
Expand Down Expand Up @@ -66,11 +67,12 @@ export class Resource implements IResource {
* information about the entity as numbers, strings or booleans
* TODO: Consider to add check/validation on attributes.
*/
private _attributes: ResourceAttributes,
attributes: ResourceAttributes,
asyncAttributesPromise?: Promise<ResourceAttributes>
) {
this._attributes = attributes;
this.asyncAttributesPending = asyncAttributesPromise != null;
this._syncAttributes = _attributes;
this._syncAttributes = this._attributes ?? {};
this._asyncAttributesPromise = asyncAttributesPromise?.then(
asyncAttributes => {
this._attributes = Object.assign({}, this._attributes, asyncAttributes);
Expand All @@ -92,15 +94,15 @@ export class Resource implements IResource {
);
}

return this._attributes;
return this._attributes ?? {};
}

/**
* Returns a promise that will never be rejected. Resolves when all async attributes have finished being added to
* this Resource's attributes. This is useful in exporters to block until resource detection
* has finished.
*/
async waitForAsyncAttributes(): Promise<void> {
async waitForAsyncAttributes?(): Promise<void> {
if (this.asyncAttributesPending) {
await this._asyncAttributesPromise;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import * as fs from 'fs/promises';
import { promises as fs } from 'fs';
import { execAsync } from './execAsync';
import { diag } from '@opentelemetry/api';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import * as fs from 'fs/promises';
import { promises as fs } from 'fs';
import { diag } from '@opentelemetry/api';

export async function getMachineId(): Promise<string> {
Expand Down
17 changes: 3 additions & 14 deletions packages/opentelemetry-resources/test/Resource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ describe('Resource', () => {
for (const resource of [resourceResolve, resourceReject]) {
assert.ok(resource.asyncAttributesPending);
await clock.nextAsync();
await resource.waitForAsyncAttributes();
await resource.waitForAsyncAttributes?.();
assert.ok(!resource.asyncAttributesPending);
}
});
Expand All @@ -174,7 +174,7 @@ describe('Resource', () => {
asyncAttributes
);

await resource.waitForAsyncAttributes();
await resource.waitForAsyncAttributes?.();
assert.deepStrictEqual(resource.attributes, {
sync: 'fromsync',
// async takes precedence
Expand Down Expand Up @@ -266,7 +266,7 @@ describe('Resource', () => {
const debugStub = sinon.spy(diag, 'debug');

const resource = new Resource({}, Promise.reject(new Error('rejected')));
await resource.waitForAsyncAttributes();
await resource.waitForAsyncAttributes?.();

assert.ok(
debugStub.calledWithMatch(
Expand Down Expand Up @@ -325,10 +325,6 @@ describe('Resource', () => {
const resource = Resource.EMPTY;
const oldResource = new Resource190({ fromold: 'fromold' });

//TODO: find a solution for ts-ignore

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
const mergedResource = resource.merge(oldResource);

assert.strictEqual(mergedResource.attributes['fromold'], 'fromold');
Expand All @@ -339,19 +335,12 @@ describe('Resource', () => {
{},
Promise.resolve({ fromnew: 'fromnew' })
);

const oldResource = new Resource190({ fromold: 'fromold' });

//TODO: find a solution for ts-ignore

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
const mergedResource = resource.merge(oldResource);

assert.strictEqual(mergedResource.attributes['fromold'], 'fromold');

await mergedResource.waitForAsyncAttributes?.();

assert.strictEqual(mergedResource.attributes['fromnew'], 'fromnew');
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import * as sinon from 'sinon';
import * as assert from 'assert';
import * as fs from 'fs/promises';
import { promises as fs } from 'fs';
import { PromiseWithChild } from 'child_process';
import * as util from '../../../../src/platform/node/machine-id/execAsync';
import { getMachineId } from '../../../../src/platform/node/machine-id/getMachineId-bsd';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import * as sinon from 'sinon';
import * as assert from 'assert';
import * as fs from 'fs/promises';
import { promises as fs } from 'fs';

import { getMachineId } from '../../../../src/platform/node/machine-id/getMachineId-linux';

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { Resource, Detector, ResourceDetectionConfig } from '../../src';
import * as assert from 'assert';

// DO NOT MODIFY THIS DETECTOR: Previous detectors used Resource as IResource did not yet exist.
// If compilation fails at this point then the changes made are breaking.
class RegressionTestResourceDetector_1_9_1 implements Detector {
async detect(_config?: ResourceDetectionConfig): Promise<Resource> {
return Resource.empty();
}
}

describe('Regression Test @opentelemetry/[email protected]', () => {
it('constructor', () => {
assert.ok(new RegressionTestResourceDetector_1_9_1());
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,21 @@ export class SimpleSpanProcessor implements SpanProcessor {
// Avoid scheduling a promise to make the behavior more predictable and easier to test
if (span.resource.asyncAttributesPending) {
const exportPromise = (span.resource as Resource)
.waitForAsyncAttributes()
.waitForAsyncAttributes?.()
.then(
() => {
this._unresolvedExports.delete(exportPromise);
if (exportPromise != null) {
this._unresolvedExports.delete(exportPromise);
}
return doExport();
},
err => globalErrorHandler(err)
);

// store the unresolved exports
this._unresolvedExports.add(exportPromise);
if (exportPromise != null) {
this._unresolvedExports.add(exportPromise);
}
} else {
void doExport();
}
Expand Down
Loading

0 comments on commit 44d6a51

Please sign in to comment.