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

integration: fix missing traces #572

Merged
merged 7 commits into from
Jun 18, 2020
Merged

integration: fix missing traces #572

merged 7 commits into from
Jun 18, 2020

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jun 17, 2020

Currently, there are a couple of issues that prevent traces emitted by
the proxy's integration tests from being reported correctly. Many of our
tests begin with:

let _ = trace_init();

This doesn't actually do what we want, for two reasons:

  • trace_init doesn't actually set a trace subscriber as the default,
    it just constructs one,
  • let _ drops the value immediately.

So, this code is constructing a subscriber and then immediately dropping
it, which does nothing.

In practice, we mostly don't notice this, since the test support
servers, clients, and proxy run in separate threads with their own
trace subscribers, which are set up correctly, and very few of the tests
themselves actually emit traces. So even though this does the wrong
thing, everything basically works. However, if the dedicated test
support threads are removed and the test support stuff moved to spawned
tasks in the test functions, all the traces go away.

This PR fixes that by making trace_init actually set a default
subscriber, adding a new trace_subscriber function that just
constructs the subscriber and level handle (which is necessary to run a
test proxy), and changing the tests so they don't drop their guards
until the test ends.

Signed-off-by: Eliza Weisman [email protected]

@hawkw hawkw requested review from olix0r, kleimkuhler and a team June 17, 2020 19:20
@hawkw hawkw self-assigned this Jun 17, 2020
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

There are a lot of double semicolons in discovery.rs and one in profile.rs. Locally it runs fine without them, so unless I'm missing something those can be removed.

linkerd/app/integration/src/client.rs Outdated Show resolved Hide resolved
hawkw and others added 5 commits June 17, 2020 16:14
@hawkw hawkw force-pushed the eliza/fix-missing-traces branch from 5ac1095 to 9808a2e Compare June 17, 2020 23:16
@olix0r olix0r merged commit eb52fd9 into main Jun 18, 2020
@olix0r olix0r deleted the eliza/fix-missing-traces branch June 18, 2020 00:48
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Jun 23, 2020
This release primarily features an upgrade of the proxy's underlying
Tokio runtime and its related libraries. We've observed lower latencies
in initial benchmarks, but further testing and burn-in is warranted.

Also, the proxy now honors the `LINKERD_PROXY_LOG_FORMAT=json`
configuration to enable JSON-formatted logging.

---

* Add a CODEOWNERS (linkerd/linkerd2-proxy#558)
* Fix shellcheck issues in shell scripts (linkerd/linkerd2-proxy#554)
* update the proxy to use std::future and Tokio 0.2 (linkerd/linkerd2-proxy#568)
* Prune unused dependencies (linkerd/linkerd2-proxy#569)
* Support LINKERD_PROXY_LOG_FORMAT=json (linkerd/linkerd2-proxy#500)
* Change docs references from "master" to "main" (linkerd/linkerd2-proxy#571)
* Upgrade tokio-rustls & webpki. (linkerd/linkerd2-proxy#570)
* Makefile: Add shellcheck recipe (linkerd/linkerd2-proxy#555)
* Update proxy-api dependencies (linkerd/linkerd2-proxy#573)
* integration: fix missing traces (linkerd/linkerd2-proxy#572)
* Update Rust to 1.44.0 (linkerd/linkerd2-proxy#574)
* Use async/await to simplify connection-accept task (linkerd/linkerd2-proxy#575)
* Update Rust to 1.44.1 (linkerd/linkerd2-proxy#576)
* outbound: Split HTTP endpoint builder (linkerd/linkerd2-proxy#578)
* Simplify protocol detection with async/await (linkerd/linkerd2-proxy#577)
* Pin proxy-api at v0.1.13 (linkerd/linkerd2-proxy#579)
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Jun 24, 2020
This release primarily features an upgrade of the proxy's underlying
Tokio runtime and its related libraries. We've observed lower latencies
in initial benchmarks, but further testing and burn-in is warranted.

Also, the proxy now honors the `LINKERD_PROXY_LOG_FORMAT=json`
configuration to enable JSON-formatted logging.

---

* Add a CODEOWNERS (linkerd/linkerd2-proxy#558)
* Fix shellcheck issues in shell scripts (linkerd/linkerd2-proxy#554)
* update the proxy to use std::future and Tokio 0.2 (linkerd/linkerd2-proxy#568)
* Prune unused dependencies (linkerd/linkerd2-proxy#569)
* Support LINKERD_PROXY_LOG_FORMAT=json (linkerd/linkerd2-proxy#500)
* Change docs references from "master" to "main" (linkerd/linkerd2-proxy#571)
* Upgrade tokio-rustls & webpki. (linkerd/linkerd2-proxy#570)
* Makefile: Add shellcheck recipe (linkerd/linkerd2-proxy#555)
* Update proxy-api dependencies (linkerd/linkerd2-proxy#573)
* integration: fix missing traces (linkerd/linkerd2-proxy#572)
* Update Rust to 1.44.0 (linkerd/linkerd2-proxy#574)
* Use async/await to simplify connection-accept task (linkerd/linkerd2-proxy#575)
* Update Rust to 1.44.1 (linkerd/linkerd2-proxy#576)
* outbound: Split HTTP endpoint builder (linkerd/linkerd2-proxy#578)
* Simplify protocol detection with async/await (linkerd/linkerd2-proxy#577)
* Pin proxy-api at v0.1.13 (linkerd/linkerd2-proxy#579)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants