-
Notifications
You must be signed in to change notification settings - Fork 36
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(collector): added support for @elastic/elasticsearch v8 #707
feat(collector): added support for @elastic/elasticsearch v8 #707
Conversation
aeffbd7
to
a880e07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work and thanks for going the extra mile to stay on top of new versions.
// eslint-disable-next-line no-useless-concat | ||
`@elastic/elasticsearch@${version}/` + `instrumentation flavor: ${instrumentationFlavor}`, | ||
function () { | ||
const indiceKey = version === 'latest' ? 'Indices.refresh' : 'indices.refresh'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
teeny tiny typo nitpick: is that meant to read indicesKey? Or indexKey?
const parsedConnectionUrl = new URL(connectionString); | ||
span.data.elasticsearch.address = parsedConnectionUrl.hostname; | ||
span.data.elasticsearch.port = parsedConnectionUrl.port; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: We could potentially shave off some CPU cycles by moving the new URL(...)
into the monkey-patched es.Client = function InstanaClient() {
function I think. If I am not mistaken, the way it is implemented now, we parse the URL on every request, although it is always the same connectionString
value.
OTOH, users might instantiate multiple instances of es.Client
, talking to different ElasticSearch clusters/servers, I guess? Could keeping only one connectionString
might lead to scenarios where we record the wrong target address/port for some calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will put the URL parsing to the patched client override for now.
OTOH, users might instantiate multiple instances of es.Client
Yes totally true. But I think there are a lot of instrumentations which ignore this use case and store data in global variables inside the instrumentations. I'd like to ignore this case.
Bumps [@elastic/elasticsearch](https://github.com/elastic/elasticsearch-js) from 7.17.0 to 8.6.0. - [Release notes](https://github.com/elastic/elasticsearch-js/releases) - [Changelog](https://github.com/elastic/elasticsearch-js/blob/main/docs/changelog.asciidoc) - [Commits](elastic/elasticsearch-js@v7.17.0...v8.6.0) --- updated-dependencies: - dependency-name: "@elastic/elasticsearch" dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
3f37db1
to
5d255b6
Compare
Bumps @elastic/elasticsearch from 7.17.0 to 8.6.0.
Release notes
Sourced from
@elastic/elasticsearch
's releases.Changelog
Sourced from
@elastic/elasticsearch
's changelog.... (truncated)
Commits
281ac00
[8.6] Add release notes for 8.6.00f0c600
[8.6] Bump@elastic/transport
to 8.3.138e4b23
Add a changelog for 8.5.004634af
Update all docs refs to 'current' instead of 'master'f79f4e8
Remove unnecessary ts-expect-error560dfd3
Fix docs URLs to use 'current' instead of 'master'ebbc296
Bumps to version 8.6.0 (#1762)6ccdab5
Add changelog for 8.4.08f9ed67
Update APIs to 8.5.0-SNAPSHOT4ebffbc
Bumps to version 8.5.0Maintainer changes
This version was pushed to npm by sethmlarson, a new releaser for
@elastic/elasticsearch
since your current version.You can trigger a rebase of this PR by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)