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

Add metrics when the connections limit is set #38161

Merged
merged 1 commit into from Jan 15, 2024

Conversation

cescoffier
Copy link
Member

When the HTTP connection limit is set:

  • expose a new gauge with the max number of connections
  • expose a new gauge with the current number of connections
  • expose a new counter with the number of rejected connections

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 12, 2024

/cc @brunobat (micrometer), @ebullient (micrometer)

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

I imagine the maxConnections is the constant you were talking in the morning...
Do you thing you can add a test?

@quarkus-bot

This comment has been minimized.

@cescoffier
Copy link
Member Author

@brunobat I can try, but it's tricky. Our test frameworks do not use persistent connections, so we will have only one connection reported. If I want to test the number of rejections, I would need to create many connections and execute them concurrently. This is the best recipe for flakiness, as the level of concurrency and execution depends on the machine executing.

I manually tested it locally because browsers, unlike our test framework, use persistent connections (and the dev UI is opening 5 connections! (I need to investigate that with @phillip-kruger when he is back)).

@brunobat
Copy link
Contributor

Ok @cescoffier. Worst case, a test with a single connection is fine because if we test the metric doesn't go away accidentally it's something already. If you add an issue for the more complex work, I'm fine merging this now.

When the HTTP connection limit is set:

- expose a new gauge with the max number of connections
- expose a new gauge with the current number of connections
- expose a new counter with the number of rejected connections
@cescoffier
Copy link
Member Author

@brunobat I accepted the challenge and implemented a test.
It should be fine on most systems....

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

thanks @cescoffier

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 15, 2024

Failing Jobs - Building 31474fd

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 🔍
JVM Tests - JDK 21 Build Failures Logs Raw logs 🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: integration-tests/elasticsearch-java-client integration-tests/elasticsearch-rest-client integration-tests/hibernate-orm-tenancy/connection-resolver and 3 more

📦 integration-tests/elasticsearch-java-client

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-elasticsearch-java-client: I/O Error

📦 integration-tests/elasticsearch-rest-client

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-elasticsearch-rest-client: I/O Error

📦 integration-tests/hibernate-orm-tenancy/connection-resolver

io.quarkus.it.hibernate.multitenancy.inventory.HibernateNamedPersistenceUnitTest.testGetPlanesDefaultTenant line 21 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code is <200> but was <500>.

	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
	at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)

io.quarkus.it.hibernate.multitenancy.inventory.HibernateNamedPersistenceUnitTest.testGetPlanesTenantMycompany line 31 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code is <200> but was <500>.

	at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
	at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)

📦 integration-tests/hibernate-search-orm-elasticsearch-tenancy

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-hibernate-search-orm-elasticsearch-tenancy: I/O Error

📦 integration-tests/hibernate-search-orm-opensearch

io.quarkus.it.hibernate.search.orm.opensearch.devservices.HibernateSearchOpenSearchDevServicesEnabledImplicitlyTest.testDevServicesProperties - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.elasticsearch.restclient.common.deployment.DevServicesElasticsearchProcessor#startElasticsearchDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/opensearchproject/opensearch:2.9.0
	at io.quarkus.elasticsearch.restclient.common.deployment.DevServicesElasticsearchProcessor.startElasticsearchDevService(DevServicesElasticsearchProcessor.java:108)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:849)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:256)

📦 integration-tests/logging-gelf

Failed to execute goal io.fabric8:docker-maven-plugin:0.43.4:start (docker-start) on project quarkus-integration-test-logging-gelf: I/O Error

@cescoffier cescoffier merged commit e642b6a into quarkusio:main Jan 15, 2024
50 of 51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Jan 15, 2024
@cescoffier cescoffier deleted the metrics-connection-rejection branch January 17, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants