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: Added instrumentation for ElasticSearch #1785

Merged
merged 22 commits into from Oct 2, 2023

Conversation

mrickard
Copy link
Member

@mrickard mrickard commented Sep 15, 2023

Description

Adds support for ElasticSearch instrumentation, for v8.0 and up

How to Test

npm run versioned elastic
npm run unit to pick up unit tests around the query parser.

Related Issues

Closes #1757
Closes NR-150258

@mrickard mrickard changed the title Nr 150258/support elasticsearch feat: Added instrumentation for ElasticSearch Sep 15, 2023
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #1785 (fa870b2) into main (9ca78ae) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1785      +/-   ##
==========================================
+ Coverage   96.84%   96.85%   +0.01%     
==========================================
  Files         198      199       +1     
  Lines       38755    38901     +146     
==========================================
+ Hits        37531    37677     +146     
  Misses       1224     1224              
Flag Coverage Δ
integration-tests-16.x 78.86% <100.00%> (+0.01%) ⬆️
integration-tests-18.x 79.13% <100.00%> (+0.01%) ⬆️
integration-tests-20.x 79.13% <100.00%> (+<0.01%) ⬆️
unit-tests-16.x ?
unit-tests-18.x 91.36% <85.61%> (-0.03%) ⬇️
unit-tests-20.x 91.36% <85.61%> (-0.03%) ⬇️
versioned-tests-16.x 75.60% <93.83%> (-0.02%) ⬇️
versioned-tests-18.x 75.60% <93.83%> (-0.02%) ⬇️
versioned-tests-20.x 75.61% <93.83%> (-0.02%) ⬇️

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

Files Coverage Δ
lib/instrumentation/@elastic/elasticsearch.js 100.00% <100.00%> (ø)
lib/instrumentations.js 100.00% <100.00%> (ø)
lib/shim/datastore-shim.js 99.41% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bizob2828 bizob2828 self-assigned this Sep 18, 2023
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

It's a good start but I think the instrumentation needs simplified a bit and the tests need to focus more on the different conditions of searching.

lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/versioned/elastic/elasticsearch.tap.js Outdated Show resolved Hide resolved
test/versioned/elastic/elasticsearch.tap.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
test/versioned/elastic/elasticsearch.tap.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
test/versioned/elastic/elasticsearch.tap.js Show resolved Hide resolved
THIRD_PARTY_NOTICES.md Outdated Show resolved Hide resolved
@mrickard mrickard force-pushed the NR-150258/support-elasticsearch branch 2 times, most recently from 6e2a355 to a854997 Compare September 22, 2023 15:32
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

overall this is looking great. Just a few comments/suggestions around some code/tests/helpers etc

lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
test/versioned/elastic/elasticsearch.tap.js Outdated Show resolved Hide resolved
test/versioned/elastic/dbtools.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved

const suffix = actions[params.method]
path.forEach((segment, idx) => {
const prev = idx - 1
Copy link
Member

Choose a reason for hiding this comment

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

I worry that someone could create an index as _search or name an index with _. In that case we're naming the collection any. It appears that's not allowed but we aren't handling that at all

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's explicitly prohibited for path index names to start with _, -, or +, and for path parameters to include /, among other punctuation characters. https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-create-index.html

Copy link
Member Author

Choose a reason for hiding this comment

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

OK--I've updated that to handle index creation separately.

mrickard and others added 18 commits September 25, 2023 15:37
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
…om connect to request, fixed calling the inContext method by calling with ctx instead of bind
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
…metric host name assertions

Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
@mrickard mrickard force-pushed the NR-150258/support-elasticsearch branch from 3afaa55 to 685e9ce Compare September 25, 2023 19:37
test/versioned/elastic/elasticsearch.tap.js Outdated Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Show resolved Hide resolved
lib/instrumentation/@elastic/elasticsearch.js Outdated Show resolved Hide resolved
test/versioned/elastic/elasticsearch.tap.js Outdated Show resolved Hide resolved
…ndex creation without manipulating index name.

Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
…ntained

Signed-off-by: mrickard <maurice@mauricerickard.com>
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

Nice test additions

@mrickard mrickard merged commit a748b84 into newrelic:main Oct 2, 2023
24 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed Oct 2, 2023
@github-actions github-actions bot mentioned this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

Add instrumentation for ElasticSearch
2 participants