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

Minor tidying of test to reduce line count and log chatter #45989

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

holly-cummins
Copy link
Contributor

I'd independently implemented #45960, because the MetricsTest was failing consistently if the execution order was different (a side effect of #34681). I only spotted the change had been made when I went to do my PR.

Looking at the changes, I think mine has a few minor advantages:

  • The reset code is a bit shorter
  • The code that asserts how many spans there are only prints out the spans in the case where there's an error, and it also fails faster on the error path

As discussed in #45955, the pattern is that the await is waiting for "have we got enough spans that there's no point waiting any more?", and then the "have we got the right number of spans?" is handled in an assertion. That gives a more useful error message, which in this case includes the listing of what spans we got. It also means that the await() doesn't wait for 4 spans to turn into 2 spans, since that's not likely to happen.

The spans can be kind of long, so we'd only want them in the log if there's a failure. This was my failure:

MetricsTest.directCounterTest:57 The spans are [{spanId=ee9a6cd667025590, traceId=1472822cdfb4d18d3aafd993e30d4b28, name=span2, kind=INTERNAL, ended=true, parentSpanId=7cd2b74c23760b0f, parent_spanId=7cd2b74c23760b0f, parent_traceId=1472822cdfb4d18d3aafd993e30d4b28, parent_remote=false, parent_valid=true, resource_host.name=hcummins-mac, resource_service.name=opentelemetry-integration-test, resource_service.version=999-SNAPSHOT, resource_telemetry.sdk.language=java, resource_telemetry.sdk.name=opentelemetry, resource_telemetry.sdk.version=1.42.1, resource_webengine.name=Quarkus, resource_webengine.version=999-SNAPSHOT}, {spanId=7cd2b74c23760b0f, traceId=1472822cdfb4d18d3aafd993e30d4b28, name=span1, kind=INTERNAL, ended=true, parentSpanId=0000000000000000, parent_spanId=0000000000000000, parent_traceId=00000000000000000000000000000000, parent_remote=false, parent_valid=false, resource_host.name=hcummins-mac, resource_service.name=opentelemetry-integration-test, resource_service.version=999-SNAPSHOT, resource_telemetry.sdk.language=java, resource_telemetry.sdk.name=opentelemetry, resource_telemetry.sdk.version=1.42.1, resource_webengine.name=Quarkus, resource_webengine.version=999-SNAPSHOT}, {spanId=249d1eeeb62805a3, traceId=72e18ae75e2713b8ea08aec1b37574a5, name=GET /direct-metrics, kind=SERVER, ended=true, parentSpanId=0000000000000000, parent_spanId=0000000000000000, parent_traceId=00000000000000000000000000000000, parent_remote=false, parent_valid=false, attr_http.response.body.size=26, attr_url.scheme=http, attr_server.address=localhost, attr_server.port=8081, attr_client.address=127.0.0.1, attr_user_agent.original=Apache-HttpClient/4.5.14 (Java/21.0.5), attr_url.path=/direct-metrics, attr_code.namespace=io.quarkus.it.opentelemetry.SimpleResource, attr_http.response.status_code=200, attr_http.request.method=GET, attr_code.function=directTraceWithMetrics, attr_http.route=/direct-metrics, resource_host.name=hcummins-mac, resource_service.name=opentelemetry-integration-test, resource_service.version=999-SNAPSHOT, resource_telemetry.sdk.language=java, resource_telemetry.sdk.name=opentelemetry, resource_telemetry.sdk.version=1.42.1, resource_webengine.name=Quarkus, resource_webengine.version=999-SNAPSHOT}, {spanId=233b28985160f31f, traceId=4afd45d9e1b831a1cc08dafcfbca6dcc, name=GET /direct-metrics, kind=SERVER, ended=true, parentSpanId=0000000000000000, parent_spanId=0000000000000000, parent_traceId=00000000000000000000000000000000, parent_remote=false, parent_valid=false, attr_http.response.body.size=26, attr_url.scheme=http, attr_server.address=localhost, attr_server.port=8081, attr_client.address=127.0.0.1, attr_user_agent.original=Apache-HttpClient/4.5.14 (Java/21.0.5), attr_url.path=/direct-metrics, attr_code.namespace=io.quarkus.it.opentelemetry.SimpleResource, attr_http.response.status_code=200, attr_http.request.method=GET, attr_code.function=directTraceWithMetrics, attr_http.route=/direct-metrics, resource_host.name=hcummins-mac, resource_service.name=opentelemetry-integration-test, resource_service.version=999-SNAPSHOT, resource_telemetry.sdk.language=java, resource_telemetry.sdk.name=opentelemetry, resource_telemetry.sdk.version=1.42.1, resource_webengine.name=Quarkus, resource_webengine.version=999-SNAPSHOT}] ==> expected: <2> but was: <4>

Copy link

quarkus-bot bot commented Jan 30, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 0091db3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@holly-cummins
Copy link
Contributor Author

@brunobat are you able to look at this?

@brunobat
Copy link
Contributor

brunobat commented Feb 3, 2025

We need to do this change elsewhere...
Will create a PR later.

@brunobat brunobat merged commit b3e5f84 into quarkusio:main Feb 3, 2025
20 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Feb 3, 2025
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