Work on #62,#63,#65, closes #117 - Splitted default config, added config for service graph and http #149
Conversation
…- Splitted default config, added config for service graph and http
There was a problem hiding this comment.
Reviewed 11 of 47 files at r1.
Reviewable status: 11 of 47 files reviewed, 6 unresolved discussions (waiting on @JonasKunz)
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/instrumentation/http/HttpOutMetricTest.java, line 28 at r1 (raw file):
public static final String GOOGLE_HOST = "www.google.com"; public static final String GOOGLE_SEARCH_URL = "https://www.google.com/search?q=hello";
Can we replace google by a wiremock endpoint when we have that dependency. Then we can specify the expected response independent of google.
inspectit-ocelot-agent/src/system-test/java/rocks/inspectit/ocelot/instrumentation/special/remote/ServletApiContextPropagationTest.java, line 221 at r1 (raw file):
String correlHeader = urlConnection.getHeaderField("Correlation-Context"); assertThat(correlHeader).contains("up_propagated=" + Math.PI + ";type=d");
Why was this changed to contains? There shouldn't be more data in the header at that point, right?
inspectit-ocelot-core/build.gradle, line 57 at r1 (raw file):
task generateConfigFilesList {
See comment in InspectitEnvironment - then we can remove this task
inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/InspectitEnvironment.java, line 246 at r1 (raw file):
private static PropertiesPropertySource loadAgentResourceYaml(String resourcePath, String propertySourceName) { Properties result = new Properties(); try (InputStream resource = InspectitEnvironment.class.getResourceAsStream("yamls.txt")) {
In my oppinion we don't need this lookup using the yamls.txtbecause Spring provides a utility for looking up resources - so we can prevent another gradle task:
Resource[] ... = new PathMatchingResourcePatternResolver(InspectitEnvironment.class.getClassLoader()).getResources("classpath:rocks/inspectit/ocelot/core/config/" + resourcePath + "/**/*.yml")
inspectit-ocelot-core/src/main/resources/rocks/inspectit/ocelot/core/config/default/basics.yml, line 1 at r1 (raw file):
inspectit:
Can we move all resources (the default and fallback directory) back to the root level or is there any specific reason for this change? Imo it is cleaner when we have it there instead of this nested hirachy.
inspectit-ocelot-core/src/main/resources/rocks/inspectit/ocelot/core/config/default/instrumentation/method-duration-sensor.yml, line 6 at r1 (raw file):
instrumentation: # # specifying instrumentation scopes which are used by rules for targeting classes and methods
I think we can remove this sample now. We will anyway document all of this in the docu
There was a problem hiding this comment.
Reviewable status: 11 of 47 files reviewed, 6 unresolved discussions (waiting on @JonasKunz)
There was a problem hiding this comment.
Reviewed 43 of 47 files at r1, 5 of 5 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
The PR is opened so that you can laready checkout and test the service graph.
I will add system tests for the http metrics on wednesday.
This change is