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(plugin): pg-pool plugin implementation #501

Merged
merged 14 commits into from
Dec 23, 2019

Conversation

xiao-lix
Copy link
Contributor

@xiao-lix xiao-lix commented Nov 7, 2019

Which problem is this PR solving?

Short description of the changes

  • Implement plugin-postgres: pg-pool

@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #501 into master will decrease coverage by 1.77%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #501      +/-   ##
==========================================
- Coverage   90.52%   88.74%   -1.78%     
==========================================
  Files         198      195       -3     
  Lines        9718     9790      +72     
  Branches      882      902      +20     
==========================================
- Hits         8797     8688     -109     
- Misses        921     1102     +181
Impacted Files Coverage Δ
...es/opentelemetry-exporter-collector/src/version.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-core/src/common/version.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-core/test/utils/url.test.ts 50% <0%> (-50%) ⬇️
...pentelemetry-core/test/internal/validators.test.ts 50% <0%> (-50%) ⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts 55.55% <0%> (-44.45%) ⬇️
...lemetry-core/test/trace/ProbabilitySampler.test.ts 56.52% <0%> (-43.48%) ⬇️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 60% <0%> (-40%) ⬇️
...s/opentelemetry-core/test/context/B3Format.test.ts 63.39% <0%> (-36.61%) ⬇️
...ges/opentelemetry-core/test/trace/NoopSpan.test.ts 63.63% <0%> (-36.37%) ⬇️
...s/opentelemetry-core/test/trace/tracestate.test.ts 65.06% <0%> (-34.94%) ⬇️
... and 31 more


export enum AttributeNames {
COMPONENT = 'component',
}
Copy link
Member

Choose a reason for hiding this comment

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

Are any of the attributes from pg applicable 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.

yes, will fix this.

}

// Else a callback was provided, so just return the result
span.end();
Copy link
Member

Choose a reason for hiding this comment

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

For this case, I think this span will be ended immediately -- before the network calls finish. For callbacks scenarios, I think we should patch the callback and end the span (and set the corresponding status) once the callback is called. Similar to pg plugin utils.patchCallback

@mayurkale22
Copy link
Member

@open-telemetry/javascript-approvers Please review this one when you get a chance.

@mayurkale22
Copy link
Member

@open-telemetry/javascript-approvers: If someone has knowledge of pg-pool (I don't), I'd love another review on this before merging.

@mayurkale22 mayurkale22 requested a review from obecny as a code owner December 9, 2019 19:00
@mayurkale22 mayurkale22 added Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) and removed Awaiting reviewer feedback labels Dec 12, 2019
@mayurkale22 mayurkale22 requested a review from dyladan as a code owner December 17, 2019 17:55
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Approving, but added a couple question comments.

}

protected patch(): typeof pgPoolTypes {
if (this._moduleExports.prototype.connect) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this if statement necessary? Is there any case where connect would be missing?

static readonly COMPONENT = 'pg-pool';
static readonly DB_TYPE = 'sql';

readonly supportedVersions = ['^2.0.7'];
Copy link
Member

Choose a reason for hiding this comment

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

Do we not support 2.0 versions below 2.0.7?

@mayurkale22
Copy link
Member

@xiao-lix Could you please address last round of comments, I think this is good to go.

@mayurkale22 mayurkale22 merged commit 1a2d926 into open-telemetry:master Dec 23, 2019
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request(plugin-postgres): pg-pool
6 participants