Fix double count ssl connection metrics#9594
Merged
clintropolis merged 4 commits intoapache:masterfrom Apr 4, 2020
Merged
Conversation
jihoonson
reviewed
Apr 2, 2020
| parser_fmt.add_argument('--header', action='store_true', help='Include header row for formats "csv" and "tsv"') | ||
| parser_fmt.add_argument('--tsv-delimiter', type=str, default='\t', help='Delimiter for format "tsv"') | ||
| parser_oth.add_argument('--context-option', '-c', type=str, action='append', help='Set context option for this connection, see https://docs.imply.io/on-prem/query-data/sql for options') | ||
| parser_oth.add_argument('--context-option', '-c', type=str, action='append', help='Set context option for this connection, see https://druid.apache.org/docs/latest/querying/sql.html#connection-context for options') |
| new InputStreamResponseHandler() | ||
| ); | ||
| // sad | ||
| Thread.sleep(100); |
Contributor
There was a problem hiding this comment.
Maybe there is some better way for waiting here rather than sleeping.. Maybe we can do like
while (jsm.getActiveConnections() == 0) {
Thread.sleep(100);
}
Assert.assertEquals(1, jsm.getActiveConnections());
go.get();
Member
Author
|
Thanks for review @jihoonson! |
clintropolis
added a commit
to clintropolis/druid
that referenced
this pull request
Apr 4, 2020
* fix double counted jetty/numOpenConnections metric for ssl connections * tests * more better * style
himanshug
pushed a commit
that referenced
this pull request
Apr 5, 2020
Merged
JulianJaffePinterest
pushed a commit
to JulianJaffePinterest/druid
that referenced
this pull request
Jun 12, 2020
* fix double counted jetty/numOpenConnections metric for ssl connections * tests * more better * style
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes an issue where the
jetty/numOpenConnectionsmetric is double counted for SSL connections due to the way allConnectionFactoryinstances are wrapped inJettyMonitoringConnectionFactoryinJettyServerModuleinitialization, instead of what I believe to be the correct usage of only monitoring the defaultConnectionFactoryfor theServerConnector.The TLS
ServerConnectoris constructed with 2ConnectionFactoryinstances:It is my understanding (and observation using the debugger) that both create a connection, the
SslConnectionFactoryto un-encrypt the request connection, and theHttpConnectionFactoryto do HTTP stuff to the now un-encrypted connection. However, since both of these were prior to this PR wrapped inJettyMonitoringConnectionFactory, a single TLS connection would count as 2 active connections, despite only 1 request being made. This PR modifiesJettyServerModuleto now only monitor theConnectionFactorywith the same protocol as the defaultServerConnectorprotocol to correctly single count the connection.The added test
JettyTest.testNumConnectionsMetricHttpsfails without the modifications toJettyServerModule. The keystore/truststore in the test resources were generated in the same manner as used in the integration tests.This PR has:
Key changed/added classes in this PR
JettyServerModule