[Debugger] Fix log snapshot tests #5600
Conversation
|
|
Ensure that the probe sent to the tracer contains a `capture` object, when `captureSnapshot` is set to `true`. This makes the test probe look more like a real probe, since a tracer might expect this object to be present (the Node.js tracer expects this).
watson
force-pushed
the
watson/DEBUG-4611/fix-tests
branch
from
71487b8 to
433de6d
Compare
| "id": "", | ||
| "version": 0, | ||
| "captureSnapshot": true, | ||
| "capture": { |
There was a problem hiding this comment.
capture object is optional and should not be a requirement for a probe definition
There was a problem hiding this comment.
As I wrote in the PR description, that test case should preferably be a separate system test. It hasn't been something that has been an issue since we launched the Node.js tracer, so I would say it's only optional in theory.
But I agree it would be good to support that, just not for this test, whose purpose is different.
There was a problem hiding this comment.
Capture object has well defined default values from the beginning, so to me it does not make sense to put this artifical capture object here
There was a problem hiding this comment.
For context, captureSnapshot: true was added to this test this week. This broke CI for the Node.js tracer, because the PR that landed it skipped the Node.js tests before landing. Then shortly after, the @bug line was added for Node.js to make Node.js CI green again. However, I think it's better, for now, to actually run the test for Node.js instead of just labeling it as a bug (which in the real world, it's not)
There was a problem hiding this comment.
captureSnapshot is required because it makes the difference between a regular log probe and a snapshot probe
There was a problem hiding this comment.
captureSnapshotis required because it makes the difference between a regular log probe and a snapshot probe
Yeah, I know that. I'm not saying adding it was a mistake. Just that it unfortunately broke Node.js CI because the issue wasn't detected before after the PR was merged
There was a problem hiding this comment.
If we add a test for this scenario, the CI will be also broken for you. How it is different than adding this @bug ?
There was a problem hiding this comment.
Adding the @bug doesn't run this test and hence hides any issues related to what this test is testing. Also, it's incorrect calling it a bug as our backend is always adding the capture object to the probe payload sent via RC. I agree that we should probably support the case where it's not there if we ever decide to remove it in some scenarios, but since we haven't done that yet, it's not a bug
There was a problem hiding this comment.
If we add a test for this scenario, we can add a @bug or @missing_feature for Node.js to that specifically, while still allowing other tests to run that tests other things.
There was a problem hiding this comment.
By default the UI happens to send :
"capture": {
"maxReferenceDepth": 3
},
but at first (as the option was not exposed) it was sending nothing.
Also the IDE plugin can also choose to send nothing.
You are not guarantee to always have this cpature object.
3 participants
Motivation
The Node.js tracer would fail if the test probe didn't look like a real probe.
Changes
Ensure that the probe sent to the tracer contains a
captureobject, whencaptureSnapshotis set totrue. This was introduced recently: #5576This makes the test probe look more like a real probe, since a tracer might expect this object to be present (the Node.js tracer expects this).
If we want to have a system test to validate that tracers can handle the situation where this object isn't present, that should be a dedicated test for that scenario.
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>], double-check that only<language>is impacted by the changebuild-XXX-imagelabel is present