-
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: added support for couchbase v4.4.4 #1466
Conversation
25c5acf
to
3723ce6
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.
Added some first comments.
); | ||
|
||
// It is difficult to get exact query from couchbase after v4.4.4 release | ||
// as they are not generating queries from JS anymore |
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.
qs: What will we loose if we only use the new instrumentation code?
We would loose two select statements for..?
This select statement looks a bit poor:
sql: 'SELECT'
In general: Please add comments before requesting review for changes like that to give attention to a critical change. I almost have overseen the change. Thanks!
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.
qs: What will we loose if we only use the new instrumentation code?
If we use only the latest instrumentation code, we will lose the ability to get SQL statements
.
In the new implementation, we won't get the queries explicitly.
This select statement looks a bit poor
In the old instrumentation, some queries are available in the spans. And trimming it for verifying in the test.
In the new instrumentation, we use the function name to verify the spans
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.
What are the SQL statements? The test only displays sql: 'SELECT'
.
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.
eg:
{
couchbase: {
hostname: 'couchbase://127.0.0.1:11210',
sql: 'CREATE INDEX `i_f72ea422-b62c-11ef-9ebd-f3edefeaf7fc` ON `d_f72ea420-b62c-11ef-9ebd-f3edefeaf7fc`.`d_f72ea421-b62c-11ef-9ebd-f3edefeaf7fc` (`name`: string)'
}
}
{
couchbase: {
hostname: 'couchbase://127.0.0.1:11210',
bucket: 'projects',
type: 'membase',
sql: 'SELECT d.* FROM `Metadata`.`Dataset` d WHERE d.DataverseName <> "Metadata"'
}
}
this is the actual span data being created using the old instrumentation logic.
Also in test,
span => expect(span.data.couchbase.sql).to.contain(options.sql || 'GET'),
we are checking the strings presence in the SQL statement.
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.
Okay. Please add a TODO for v5 to drop the instrumentation code for v443. Please add a TODO in the codebase for v5.
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.
We need to support both versions of v4 upon the release of v5, right?
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.
Yes exactly 👍 We can drop the logic for 4.4.3 in v5
packages/core/src/tracing/instrumentation/database/couchbase.js
Outdated
Show resolved
Hide resolved
packages/core/src/tracing/instrumentation/database/couchbase.js
Outdated
Show resolved
Hide resolved
); | ||
|
||
// It is difficult to get exact query from couchbase after v4.4.4 release | ||
// as they are not generating queries from JS anymore |
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.
Okay. Please add a TODO for v5 to drop the instrumentation code for v443. Please add a TODO in the codebase for v5.
91ff5e8
to
2ed7583
Compare
packages/core/src/tracing/instrumentation/database/couchbase.js
Outdated
Show resolved
Hide resolved
packages/core/src/tracing/instrumentation/database/couchbase.js
Outdated
Show resolved
Hide resolved
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
packages/core/src/tracing/instrumentation/database/couchbase.js
Outdated
Show resolved
Hide resolved
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.
added minor comment, LGTM
bcf417a
to
cef5b54
Compare
…re failing in pipeline
Co-authored-by: kirrg001 <[email protected]>
Co-authored-by: kirrg001 <[email protected]>
cef5b54
to
366cebc
Compare
refs INSTA-20554
couchnode changelog: couchbase/couchnode@v4.4.3...v4.4.4
In this update, there is code changes that affects our instrumentation.
We have instrumented
cluster.analyticsQuery
for analyticsServices,await this._cluster.analyticsQuery
:https://github.com/couchbase/couchnode/blob/e855b094cd1b0140ffefc40f32a828b9134d181c/lib/analyticsindexmanager.ts#L1039
now the core implementation changed and we have to instrument all functions separately like
conn.managementAnalyticsDataverseDrop
:https://github.com/couchbase/couchnode/blob/9e3e1f1174d5e3aa29f6499c08fafd2ccbf8dbdf/lib/analyticsindexmanager.ts#L1202C21-L1202C58