Add responseless dns requests to dns_data table. #613

noman-xg · 2022-10-10T12:08:00Z

This pull request is related to #418 which relates to including those DNS requests into the dns_data table for which a matching response was not found.

Testing Done

Verified that tests in stitcher_test.cc are passed.

noman@px-dev-docker:/pl/src/px.dev/pixie$ bazel test src/stirling/source_connectors/socket_tracer/protocols/dns/...
INFO: Analyzed 7 targets (0 packages loaded, 0 targets configured).
INFO: Found 4 targets and 3 test targets...
INFO: Elapsed time: 3.078s, Critical Path: 2.69s
INFO: 11 processes: 1 internal, 10 processwrapper-sandbox.
INFO: Build completed successfully, 11 total actions
//src/stirling/source_connectors/socket_tracer/protocols/dns:parse_test  PASSED in 0.0s
//src/stirling/source_connectors/socket_tracer/protocols/dns:stitcher_test PASSED in 0.0s
//src/stirling/source_connectors/socket_tracer/protocols/dns:types_test  PASSED in 0.0s

INFO: Build completed successfully, 11 total actions
noman@px-dev-docker:/pl/src/px.dev/pixie$ cat   /home/noman/.cache/bazel/_bazel_noman/4c31fb537ca0f31ab15bbd6a8445d3b6/execroot/px/bazel-out/k8-fastbuild/testlogs/src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher_test/test.log
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //src/stirling/source_connectors/socket_tracer/protocols/dns:stitcher_test
I20221010 11:30:13.268741  5618 env.cc:47] Started: /home/noman/.cache/bazel/_bazel_noman/4c31fb537ca0f31ab15bbd6a8445d3b6/sandbox/processwrapper-sandbox/21/execroot/px/bazel-out/k8-fastbuild/bin/src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher_test.runfiles/px/src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher_test
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from DnsStitcherTest
[ RUN      ] DnsStitcherTest.RecordOutput
[       OK ] DnsStitcherTest.RecordOutput (0 ms)
[ RUN      ] DnsStitcherTest.OutOfOrderMatching
[       OK ] DnsStitcherTest.OutOfOrderMatching (0 ms)
[----------] 2 tests from DnsStitcherTest (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 2 tests.
I20221010 11:30:13.269136  5618 env.cc:51] Shutting down

pixie-io-buildbot · 2022-10-10T12:08:02Z

Can one of the admins verify this patch?

Changed Stichframes() to include the DNS requests in dns_data table which could not be matched with a response. Signed-off-by: Noman_ali_bajwa <nmnbajwa@gmail.com>

zasgar · 2022-10-11T21:40:48Z

ok to test

…o-dns_data

Changed Bazel.build file to include dependacny for stitcher_test.

noman-xg · 2022-10-12T18:34:37Z

Hey folks, how can i see the jenkins build log. The details hyperlink takes me to google sigin to continue to Pixie labs which says "Access blocked: Pixie Labs can only be used within its organization"
@htroisi

htroisi · 2022-10-12T18:37:31Z

We're working on improving our process so that the build logs are public. But for now, I DM'd you the logs.

noman-xg · 2022-10-12T18:58:07Z

BLOCKED

Hey folks, Jenkins build is failing with this error. Any pointer on this one ?

Apply this patch to src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc? [Y/n] EXCEPTION: (PhutilConsoleStdinNotInteractiveException) The program is attempting to read user input, but stdin is being piped from some other source (not a TTY). at [<arcanist>/src/console/format.php:196] arcanist(head=stable, ref.master=85c953ebe4a6, ref.stable=b6babd9a07f4), arcanist-addons() #0 phutil_console_require_tty() called at [<arcanist>/src/console/format.php:47] #1 phutil_console_prompt(string) called at [<arcanist>/src/console/format.php:15] #2 phutil_console_confirm(string, boolean) called at [<arcanist>/src/lint/renderer/ArcanistConsoleLintRenderer.php:47] #3 ArcanistConsoleLintRenderer::promptForPatch(ArcanistLintResult, string, TempFile) called at [<arcanist>/src/workflow/ArcanistLintWorkflow.php:312] #4 ArcanistLintWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427] <<< [1] (+124,647) <exec> 124,647,614 us script returned exit code 255Unhandled hudson.AbortException: script returned exit code 255, assuming fatal error.

oazizi000 · 2022-10-12T22:33:35Z

@Noman-Ali-Bajwa I think this is because the Jenkins build expects the diff to be pre-linted. We should figure out how to get the linters to run on your local environment.

Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

ghost · 2022-10-12T22:47:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: noman-xg
Once this PR has been reviewed and has the lgtm label, please assign zasgar for approval by writing /assign @zasgar in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ghost · 2022-10-12T22:47:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: noman-xg
Once this PR has been reviewed and has the lgtm label, please assign zasgar for approval by writing /assign @zasgar in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

oazizi000

Hi @Noman-Ali-Bajwa,

Thanks so much for this preview PR! So glad to have your contribution.

This is a great start. Leaving a few comments for discussion below.

oazizi000 · 2022-10-17T06:02:57Z

src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc

+// CustomStitchFrames is a re-implementation of the existing StitchFrames with an added
+// ability to record those DNS requests as well for which we could not match any response.
+// Flag include_respless_dns_requests must be set to include such DNS requests.
+RecordsWithErrorCount<Record> CustomStitchFrames(std::deque<Frame>* req_frames,
+                                                 std::deque<Frame>* resp_frames) {


The high degree of similarity of this function with PxStitchFrames suggests we should merge them and pass in an boolean argument that selects the behavior. Otherwise there's too much duplication, which could be a problem if there's some bug fix we need to make in the future.

Also, I know you asked about this on Slack. Thanks for putting this preview out so I could get a better grasp on how much duplication there actually ends up being :)

I'll merge the two and push the changes soon.

fixed in a3e9243

oazizi000 · 2022-10-17T06:05:55Z

src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc

+  // After the external loop's lifecycle comes to an end we end up with the request deque
+  // having only those request frames which have not been consumed yet i.e. consumed = false
+  // so essentially these are the requests which could not be matched with any response frame.
+  // Hence we iterate over this request deque, add a default response to it, make a record and
+  // append those records at the end of the entries vector.
+  auto it = req_frames->begin();
+  while (it != req_frames->end()) {
+    if (!(*it).consumed) {
+      Frame default_resp_frame;
+      default_resp_frame.timestamp_ns = 99;
+      StatusOr<Record> record_status = ProcessReqRespPair(*it, default_resp_frame);
+      entries.push_back(record_status.ConsumeValueOrDie());
+    }
+    it++;
+  }
+  return {entries, error_count};


This is the new part. A couple cases that we'll probably have to handle:

What if a request can't be matched with a response because the response is still in flight? The parser could run at any time, so we we have to be ready to handle that case. We might need to build in some assumptions about how delayed a response can be.

What's the rationale on default_resp_frame.timestamp_ns = 99;? We'll have to be careful with that because it could affect latency calculations.

What should we consider as a timeout for the dns request. I think we can handle that using timestamp_ns.

I have a workaround in mind to remove this. Will change in next commit.

For (1), there's no set number, but nslookup uses a default of 2 seconds if I'm not mistaken. So that could be a starting point. Whatever we choose, it should definitely be a FLAG so we can change it later if required.

timeout added in a3e9243

Made changes to set a timeout on DNS requests. Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

oazizi000

Thanks @Noman-Ali-Bajwa for making the updates. Here are a few more on the latest version.

oazizi000 · 2022-10-19T05:26:28Z

src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc

+    // append those records at the end of the entries vector.
+    auto it = req_frames->begin();
+    while (it != req_frames->end()) {
+      if (!(*it).consumed) {


style: if (!it->consumed)

oazizi000 · 2022-10-19T05:27:53Z

src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc

+        auto current_time = std::chrono::duration_cast<std::chrono::milliseconds>(
+                                std::chrono::system_clock::now().time_since_epoch())
+                                .count();


Getting the clock can become expensive when inside a loop. Can you get the current_time a single time outside the loop and use the fixed value inside the loop. While not strictly as accurate, it should be more than sufficient for this use case.

oazizi000 · 2022-10-19T05:28:09Z

src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc

+        auto current_time = std::chrono::duration_cast<std::chrono::milliseconds>(
+                                std::chrono::system_clock::now().time_since_epoch())
+                                .count();
+        auto elapsed_seconds = current_time - (*it).timestamp_ns;


style: it->timestsamp_ns

oazizi000 · 2022-10-19T05:30:29Z

src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc

+
+        if (elapsed_seconds > FLAGS_dns_request_timeout_threshold_milliseconds) {
+          Frame default_resp_frame;
+          default_resp_frame.timestamp_ns = 99;


Still thinking about this one. At minimum, we could use the time of the timeout as the "response" time.

I'll get it out of the way today.

Changed dummy response_frame timestamp to be increment of request timestamp. Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

…y timeout threshold. Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

oazizi000

Looking good. One additional small comment. Thanks @Noman-Ali-Bajwa!

oazizi000 · 2022-10-20T14:01:30Z

src/stirling/source_connectors/socket_tracer/protocols/dns/stitcher.cc

+DEFINE_uint64(dns_request_timeout_threshold_milliseconds, 2000,
+              "Number of seconds to wait for the in-flight response of a dns request.");


Can you add, in the description, that this flag depends on include_respless_dns_requests?

fixed in ef1516a

oazizi000 · 2022-10-20T14:03:12Z

Also, feel free to take off the [PREVIEW] prefix on the PR :)

Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

bcff612

Changed Stichframes() to include the DNS requests in dns_data table which could not be matched with a response. Signed-off-by: Noman_ali_bajwa <nmnbajwa@gmail.com>

noman-xg changed the title ~~[PREVIEW] Add responseless dns requests to dns_data table. #418~~ [PREVIEW] Add responseless dns requests to dns_data table. Oct 10, 2022

htroisi requested review from oazizi000 and chengruizhe October 10, 2022 18:11

noman-xg and others added 2 commits October 12, 2022 22:21

Merge branch 'pixie-io:main' into add-dns-requests-without-response-t…

6c4638e

…o-dns_data

Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

82d82a6

Changed Bazel.build file to include dependacny for stitcher_test.

Linted the changes using arc lint.

6b95316

Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

oazizi000 reviewed Oct 17, 2022

View reviewed changes

Merged functions to remove code duplication.

a3e9243

Made changes to set a timeout on DNS requests. Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

oazizi000 reviewed Oct 19, 2022

View reviewed changes

Noman-Ali-Bajwa added 2 commits October 19, 2022 11:34

Code style fixes. Optimization.

71b4e20

Changed dummy response_frame timestamp to be increment of request timestamp. Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

Changed default_response_timestamp to request_timestamp incremented b…

80d01df

…y timeout threshold. Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

oazizi000 previously approved these changes Oct 20, 2022

View reviewed changes

Modified description of flag.

ef1516a

Signed-off-by: Noman Ali Bajwa <nmnbajwa@gmail.com>

noman-xg dismissed oazizi000’s stale review via ef1516a October 20, 2022 14:09

noman-xg changed the title ~~[PREVIEW] Add responseless dns requests to dns_data table.~~ Add responseless dns requests to dns_data table. Oct 20, 2022

oazizi000 approved these changes Oct 20, 2022

View reviewed changes

htroisi mentioned this pull request Oct 20, 2022

Modify dns_data table to record DNS requests with no response #418

Closed

copybaranaut closed this in 1beffd1 Oct 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add responseless dns requests to dns_data table. #613

Add responseless dns requests to dns_data table. #613

noman-xg commented Oct 10, 2022

pixie-io-buildbot commented Oct 10, 2022

zasgar commented Oct 11, 2022

noman-xg commented Oct 12, 2022 •

edited

Loading

htroisi commented Oct 12, 2022

noman-xg commented Oct 12, 2022 •

edited

Loading

oazizi000 commented Oct 12, 2022

ghost commented Oct 12, 2022

ghost commented Oct 12, 2022

oazizi000 left a comment

oazizi000 Oct 17, 2022

noman-xg Oct 17, 2022

noman-xg Oct 18, 2022

oazizi000 Oct 17, 2022

noman-xg Oct 17, 2022 •

edited

Loading

oazizi000 Oct 17, 2022

noman-xg Oct 18, 2022

oazizi000 left a comment

oazizi000 Oct 19, 2022

oazizi000 Oct 19, 2022

oazizi000 Oct 19, 2022

oazizi000 Oct 19, 2022

noman-xg Oct 19, 2022

oazizi000 left a comment

oazizi000 Oct 20, 2022

noman-xg Oct 20, 2022

oazizi000 commented Oct 20, 2022

		DEFINE_uint64(dns_request_timeout_threshold_milliseconds, 2000,
		"Number of seconds to wait for the in-flight response of a dns request.");

Add responseless dns requests to dns_data table. #613

Add responseless dns requests to dns_data table. #613

Conversation

noman-xg commented Oct 10, 2022

Testing Done

pixie-io-buildbot commented Oct 10, 2022

zasgar commented Oct 11, 2022

noman-xg commented Oct 12, 2022 • edited Loading

htroisi commented Oct 12, 2022

noman-xg commented Oct 12, 2022 • edited Loading

oazizi000 commented Oct 12, 2022

ghost commented Oct 12, 2022

ghost commented Oct 12, 2022

oazizi000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noman-xg Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oazizi000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oazizi000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oazizi000 commented Oct 20, 2022

noman-xg commented Oct 12, 2022 •

edited

Loading

noman-xg commented Oct 12, 2022 •

edited

Loading

noman-xg Oct 17, 2022 •

edited

Loading