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

chore: Add rules engine to match OTEL spans #2694

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

jsumners-nr
Copy link
Contributor

This PR resolves #2667.

@jsumners-nr jsumners-nr marked this pull request as ready for review October 31, 2024 18:59
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 93.78882% with 10 lines in your changes missing coverage. Please review.

Project coverage is 97.31%. Comparing base (13a627a) to head (bc271e4).
Report is 22 commits behind head on next.

Files with missing lines Patch % Lines
lib/otel/rules.js 93.78% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             next    #2694       +/-   ##
===========================================
+ Coverage   79.26%   97.31%   +18.05%     
===========================================
  Files         281      296       +15     
  Lines       44906    46597     +1691     
===========================================
+ Hits        35593    45345     +9752     
+ Misses       9313     1252     -8061     
Flag Coverage Δ
integration-tests-cjs-18.x 74.32% <ø> (?)
integration-tests-cjs-20.x 74.33% <ø> (?)
integration-tests-cjs-22.x 74.36% <ø> (?)
integration-tests-esm-18.x 49.85% <ø> (-0.03%) ⬇️
integration-tests-esm-20.x 49.85% <ø> (-0.03%) ⬇️
integration-tests-esm-22.x 49.88% <ø> (-0.03%) ⬇️
unit-tests-18.x 89.01% <93.78%> (?)
unit-tests-20.x 89.01% <93.78%> (?)
unit-tests-22.x 89.02% <93.78%> (?)
versioned-tests-18.x 79.10% <ø> (-0.07%) ⬇️
versioned-tests-20.x 79.14% <ø> (-0.04%) ⬇️
versioned-tests-22.x 79.12% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


// Based upon https://github.com/open-telemetry/opentelemetry-js/blob/8fc76896595aac912bf9e15d4f19c167317844c8/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts#L851

const test = require('node:test')
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 test even getting run? it's not in the tests folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not getting run automatically at this time. I only wanted to be able to verify the work while we hash out the structure and underlying details. My intention is to relocate it as we draw near to it being a real feature.

Copy link
Member

Choose a reason for hiding this comment

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

got it

bizob2828
bizob2828 previously approved these changes Nov 7, 2024
@bizob2828
Copy link
Member

There's a linting error

@jsumners-nr
Copy link
Contributor Author

There's a linting error

I know. I don't know why, though. How was I able to install and use the package if it isn't published? I'm going to shove a lint exception in there, but I have that fork in a state of change and don't want to switch branches. I'll get it updated when I get to a point where I can switch branches.

@bizob2828
Copy link
Member

There's a linting error

I know. I don't know why, though. How was I able to install and use the package if it isn't published? I'm going to shove a lint exception in there, but I have that fork in a state of change and don't want to switch branches. I'll get it updated when I get to a point where I can switch branches.

Hmm, let me take a closer look, we shouldn't have to do that

@bizob2828
Copy link
Member

Linting is failing because the test is in lib. I moved to a mirrored folder in test/lib/otel and the eslint rule goes away so I would just move this test file

@jsumners-nr
Copy link
Contributor Author

Linting is failing because the test is in lib. I moved to a mirrored folder in test/lib/otel and the eslint rule goes away so I would just move this test file

That's... amazing.

@jsumners-nr jsumners-nr marked this pull request as draft November 8, 2024 18:02
@bizob2828 bizob2828 marked this pull request as ready for review November 12, 2024 16:35
@jsumners-nr jsumners-nr merged commit fe3fa28 into newrelic:next Nov 12, 2024
23 checks passed
@jsumners-nr jsumners-nr deleted the issue-2667 branch November 12, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

2 participants