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

Ability to set the configuration file from the classpath of runtime attachment #2581

Merged
merged 10 commits into from Oct 12, 2022

Conversation

jeanbisutti
Copy link
Member

Fix #2576

InputStream configContentAsInputStream =
ApplicationInsights.class.getResourceAsStream("/applicationinsights.json");
ApplicationInsights.class.getResourceAsStream("/" + fileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we update smoke test for runtime attach to test this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but we would have to add the following kind of code to the current test:

  public static void main(String[] args) {
    System.setProperty("applicationinsights.runtime-attach.configuration.file", "applicationinsights-test.json");
    ApplicationInsights.attach();
    SpringApplication.run(SpringBootApp.class, args);
  }

The users would instead add a system property with -Dapplicationinsights.runtime-attach.configuration.file=applicationinsights-test.json. So, the test would not represent the typical way of configuring the file name. And so, the test would not document the standard way of configuring the file name.

Another option could be to rewrite a test to be able to add -Dapplicationinsights.runtime-attach.configuration.file=applicationinsights-test.json. However, this would require a significant time for an added code pretty simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a todo for now? it's not required for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

is there some other form of test coverage that may be simpler (but still useful)?

Copy link
Member Author

Choose a reason for hiding this comment

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

An approach could be to change the visibility of some methods to be able to test them. However, I don't recommend doing this. The major drawback of this approach is that we are testing implementation details, not a user use case. So, since the test is tied to implementation details, refactoring (changing the code without changing the behavior) could break it (because we remove a method, for example), adding a cost to test maintenance and slowing down and discouraging future refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

I created a work item for the smoke test and we can discuss if/how to implement it in the future

@trask trask merged commit f19e3f5 into main Oct 12, 2022
@trask trask deleted the ra-configuration-file branch October 12, 2022 19:12
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.

Appinsights logging for different environments
3 participants