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

src: add trace points to dns #21840

Closed
wants to merge 1 commit into from

Conversation

chinhuang007
Copy link
Contributor

Add trace points to dns under node.dns category.
Emit trace events for dns operations. Use the
nestable async events instead of deprecated ones.
Include test code to verify the trace log.

Refs: #19157

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [x ] tests and/or benchmarks are included
  • [x ] commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Jul 16, 2018
@@ -714,6 +719,9 @@ class QueryWrap : public AsyncWrap {
extra
};
const int argc = arraysize(argv) - extra.IsEmpty();
static const std::string trace_name = name_;
TRACE_EVENT_NESTABLE_ASYNC_END0("node,node.dns", trace_name.c_str(), this);
Copy link
Member

Choose a reason for hiding this comment

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

This looks odd … this means that trace_name is initialized once and not changed afterwards? What if this function gets called with instances of QueryWrap that have different names?

I think things might end up being easier if you store the original const char* itself on the class and always pass that to the trace macros. That might require the string to be given at compile time, but I’d think that’s okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. I will need to fix it.

@addaleax addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jul 16, 2018
: AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP),
channel_(channel) {
channel_(channel),
trace_name_(name) {
Copy link
Member

Choose a reason for hiding this comment

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

does this change cause any external JS API interface change? I guess not.

Copy link
Member

Choose a reason for hiding this comment

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

No, this is all internal.

'dns.resolveSrv("example.com", (err, res) => {});' +
'dns.resolvePtr("example.com", (err, res) => {});' +
'dns.resolveNaptr("example.com", (err, res) => {});' +
'dns.resolveSoa("example.com", (err, res) => {});';
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to wrap all these callbacks into common.mustCall semantic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comments. The callbacks are not significant in the trace log test case. There are existing test files that cover general dns features. The main focus here is to ensure an expected trace log is written to the trace output file. I did break the all-in-one test into individual tests so it will be easier to pinpoint the problem should an error occurs.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense, thanks. My experience showed that issues are often caught by test cases that were not designed to test such issues. Moreover the semantic which we are talking about is present in every piece of test code that has a completion handler callback. But definitely not a reason for tests to overload cases, and I am fine here.

@@ -618,6 +620,9 @@ class QueryWrap : public AsyncWrap {
int dnsclass,
int type) {
channel_->EnsureServers();
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(
"node,node.dns", trace_name_, this,
Copy link
Member

Choose a reason for hiding this comment

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

Let's make the category on these node.dns.native. Once we have the V8 trace instrinsic landed they will be paired up with the node.dns.js side.

Also, there's a macro to make the naming easier... e.g. TRACING_CATEGORY_NODE2(dns, native) expands to node,node.dns,node.dns.native to ensure we have a proper hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comments. I changed the category to node.dns.native, also started to use TRACING_CATEGORY_NODE2(dns, native).

: AsyncWrap(channel->env(), req_wrap_obj, AsyncWrap::PROVIDER_QUERYWRAP),
channel_(channel) {
channel_(channel),
trace_name_(name) {
Copy link
Member

Choose a reason for hiding this comment

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

No, this is all internal.

@@ -0,0 +1,86 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

This test should probably go into test/internet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly I are not sure the technical differences between test/internet and test/parallel. I saw dns test code in both folders. For now, I put this under test/parallel, along with other test-trace-events-xxx test files. I don't mind to move if it should go to test/internet by definition.

Copy link
Member

Choose a reason for hiding this comment

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

@chinhuang007 Generally, test that reach out to random internet servers should go into test/internet – think of it as tests that could be affected by e.g. a firewall. These tests aren’t run as part of a standard make test, though.

There are DNS tests in test/parallel, but (I think) most of those use a DNS mock implementation (e.g. test/parallel/test-dns-resolveany.js) – that’s nice, because it means that the test is fully self-contained and doesn’t do any networking outside the local host, but it also comes with some overhead when writing the test.

It’s a tradeoff, and you can choose to go either route, but if you keep the test as it is, please move it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, moved the test code to test/parallel. Thanks for your explanation!

@chinhuang007 chinhuang007 force-pushed the add_dns_trace branch 2 times, most recently from 33668dd to d7ede7a Compare July 18, 2018 00:17
@chinhuang007
Copy link
Contributor Author

What's the next step to move it forward?

@chinhuang007
Copy link
Contributor Author

If no changes are needed, can someone help start the merge/land process?

@addaleax
Copy link
Member

@chinhuang007 I think this is technically ready, but /cc @joyeecheung because https://github.com/nodejs/node/pull/21939/files#diff-5a543a2c1d958ec924db11553ecfb703 also adds string identifiers to all of the different query classes

CI: https://ci.nodejs.org/job/node-test-pull-request/16004/

@chinhuang007 chinhuang007 force-pushed the add_dns_trace branch 2 times, most recently from 849f83b to 128e866 Compare July 26, 2018 00:28
TRACING_CATEGORY_NODE2(dns, native), "lookup", req_wrap,
"hostname", TRACE_STR_COPY(*hostname),
"family",
family == AF_INET ? "ip4" : family == AF_INET6 ? "ip6" : "unspec");
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these are ip4 and ip6 instead of ipv4 and ipv6? (like the ones in GetHostByAddrWrap)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching it. Just fixed it.

@joyeecheung
Copy link
Member

It should be orthogonal to #21939 since the naming here is for time spent on (JS-API-ish) operations (measured in microseconds) instead of internal types of chunks in memory (measured in bytes).

@chinhuang007 chinhuang007 force-pushed the add_dns_trace branch 3 times, most recently from 7d401e2 to 420daf0 Compare July 27, 2018 00:04
@chinhuang007
Copy link
Contributor Author

Thanks, @joyeecheung for the review and comments. I fixed a CI problem for osx. Can someone help start a new CI?

@gireeshpunathil
Copy link
Member

@chinhuang007
Copy link
Contributor Author

The CI build problem doesn't seem related this PR. Can someone start a new CI to see if the problem is repeatable?

@yhwang
Copy link
Member

yhwang commented Aug 1, 2018

@chinhuang007
Copy link
Contributor Author

Please let me know if anything else needs to be done before this PR can be merged/landed. Thanks!

@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

@yhwang
Copy link
Member

yhwang commented Aug 16, 2018

@chinhuang007
Copy link
Contributor Author

@jasnell Thanks for starting the CI. Somehow some tests failed on unrelated build issues. I wonder if you can help merge/land it?

@jasnell
Copy link
Member

jasnell commented Aug 18, 2018

@addaleax
Copy link
Member

@joyeecheung
Copy link
Member

This PR needs a rebase against master to avoid the git failure in the CI.

Add trace points to dns under node.dns.native category.
Emit trace events for dns operations. Use the
nestable async events instead of deprecated ones.
Include test code to verify the trace log. The
trace name is stored as const char* class variable.
The test code is to check each operation in separate
sync processes.

Refs: nodejs#19157
@chinhuang007
Copy link
Contributor Author

Thanks for the CIs and comments. I just rebased and locally tested. Hope the next CI will be clean.

@joyeecheung
Copy link
Member

@addaleax
Copy link
Member

Only one unrelated FreeBSD failure remaining 😄

Resume build: https://ci.nodejs.org/job/node-test-pull-request/16619/

@chinhuang007
Copy link
Contributor Author

Just rebased and passed tests. Do we need to start a new CI, hoping no unrelated errors? Please advice what I can do to help land it.

@addaleax
Copy link
Member

addaleax commented Sep 2, 2018

@chinhuang007 Sorry, all that was needed was a ping to remind us – landed in b366de5!

@addaleax addaleax closed this Sep 2, 2018
addaleax pushed a commit that referenced this pull request Sep 2, 2018
Add trace points to dns under node.dns.native category.
Emit trace events for dns operations. Use the
nestable async events instead of deprecated ones.
Include test code to verify the trace log. The
trace name is stored as const char* class variable.
The test code is to check each operation in separate
sync processes.

Refs: #19157

PR-URL: #21840
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@refack
Copy link
Contributor

refack commented Sep 3, 2018

Since this test is in /internet/ it is not covered by our regular CI.
Daily run picked up https://ci.nodejs.org/job/node-test-commit-custom-suites/675/default/testReport/junit/(root)/test/internet_test_trace_events_dns/

Update: Fix PR #22674

@refack refack mentioned this pull request Sep 3, 2018
3 tasks
targos pushed a commit that referenced this pull request Sep 3, 2018
Add trace points to dns under node.dns.native category.
Emit trace events for dns operations. Use the
nestable async events instead of deprecated ones.
Include test code to verify the trace log. The
trace name is stored as const char* class variable.
The test code is to check each operation in separate
sync processes.

Refs: #19157

PR-URL: #21840
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Add trace points to dns under node.dns.native category.
Emit trace events for dns operations. Use the
nestable async events instead of deprecated ones.
Include test code to verify the trace log. The
trace name is stored as const char* class variable.
The test code is to check each operation in separate
sync processes.

Refs: #19157

PR-URL: #21840
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
vmarchaud added a commit to vmarchaud/node that referenced this pull request Jun 7, 2019
As implemented in nodejs#21840, dns can
emit trace events when the category is enabled. This PR just add
it to the documentation.
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 13, 2019
As implemented in nodejs#21840, dns can
emit trace events when the category is enabled. This PR just add
it to the documentation.

PR-URL: nodejs#28100
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
As implemented in #21840, dns can
emit trace events when the category is enabled. This PR just add
it to the documentation.

PR-URL: #28100
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants