Skip to content

Rework Internal Logger, support specifying absolute path for log base folder#763

Merged
dhaval24 merged 6 commits into
masterfrom
Agent/FileLogger
Nov 9, 2018
Merged

Rework Internal Logger, support specifying absolute path for log base folder#763
dhaval24 merged 6 commits into
masterfrom
Agent/FileLogger

Conversation

@dhaval24

@dhaval24 dhaval24 commented Oct 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor
  1. Abstracted Core Internal Logger into separate package shared with Agent.
  2. Deprecated AgentInternalLogger in favor of Core Internal Logger.
  3. Agent will have capability to write logs to files .
  4. Introduced ability to write logs to files in specified absolute file path.
  5. Introduced a new configuration tag for No.4 -> <BaseFolderPath>
  6. Deprecated <BaseFolderName> tag in favor of <BaseFolderPath>
  7. The File logging option would only support absolute path location.
  8. Used Files and Paths API.
  9. Clean up unused imports etc.
  10. Reworked build scripts little bit to ensure that poms are generated properly (ensure that logging module isn't present in the poms.)
  11. Added ability to configure file logger in SpringBoot starter.
  12. Fix Update InternalAgentLogger with option to write to file #752 Update SDK Logger to output to any directory #751
    Sample usage for agent:
<AgentLogger type = "FILE">
        <Level>TRACE</Level>
        <UniquePrefix>AI</UniquePrefix>
        <BaseFolderPath>C:/agent/AIAGENT</BaseFolderPath>
</AgentLogger>

Sample usage for core:

<SDKLogger type="FILE">
    <Level>TRACE</Level>
    <UniquePrefix>AI</UniquePrefix>
    <BaseFolderPath>C:/agent/AISDK</BaseFolderPath>
</SDKLogger>

sample usage for SpringBoot starter:

azure.application-insights.logger.type=file
azure.application-insights.logger.base-folder-path=C:/agent/AISDK
azure.application-insights.logger.level=trace

Note: It is essential to specify separate folder for AgentLogging and SDKLogging. This doesn't fix the logging in the AgentImplementation.java class.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public surface reviewed
  • CHANGELOG.md updated

@dhaval24
enable file logger for agent, refactor and abstract internal logger, …
111f00b 
…add capability to specify absolute path, clean up
@dhaval24 dhaval24 added this to the 2.2.1 milestone Oct 18, 2018
@dhaval24 dhaval24 self-assigned this Oct 18, 2018
@dhaval24 dhaval24 requested review from grlima and littleaj October 18, 2018 01:17
littleaj
littleaj suggested changes Oct 23, 2018
View reviewed changes

@littleaj littleaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The config elements BaseFolder and BaseFolderPath read as synonyms. I thought the design was to use BaseFolder and only root it under java.io.tmpdir if the path was relative. If it's absolute, then use that directly.

@dhaval24

dhaval24 commented Oct 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@littleaj I am not sure, if there is easy way to determine if the path is relative or absolute. Do you have any clue for that ? May be this ? https://stackoverflow.com/questions/7627049/how-to-check-whether-the-path-is-relative-or-absolute-in-java

Though I am not sure, it feels to me a bit confusing with annonymously checking if the path is relative or absolute.

@littleaj

littleaj commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

@dhaval24, I'm not sure what you mean by "anonymously checking," but yes, that's the method I had in mind. The idea was to support the current way (where everything is relative to temp) and add the functionality which supports absolute paths. I don't think we should reject relative paths since they work currently.

@dhaval24

dhaval24 commented Oct 24, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@littleaj in my modification, if you use just specify the base folder name, it would be relative to temp folder, so I believe in that way currently functionality is preserved.

@dhaval24

dhaval24 commented Nov 6, 2018

Copy link
Copy Markdown
Contributor Author

@grlima and @littleaj I have verified the change in the app-services and I can see the agent logs in file. I think this PR should be good to go, if there are no pending concerns. I think it solves the purpose :)

@littleaj

littleaj commented Nov 7, 2018

Copy link
Copy Markdown
Contributor

@dhaval24 @grlima, I still feel like having BaseFolder and BaseFolderPath is confusing. I don't think we need to introduce another config element.

@dhaval24

dhaval24 commented Nov 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks @littleaj ! I understand your concern. Is it possible for us to brainstorm a better solution together. Would you mind throwing your ideas ? One I have on top of mind is to just use BaseFolder and support absolute path and remove BaseFolderPath element. I am just not very positive about absolute path vs relative path identification at the run time (it seems to be a bit complicated to me).

What is the downside of just supporting absolute path? I would like to understand better.

I am happy to alter solution as per the combined decision. Thanks!

@dhaval24

dhaval24 commented Nov 8, 2018

Copy link
Copy Markdown
Contributor Author

@littleaj @grlima I have reworked this PR to simplify things. Please take a look and let me know if it is good to go.

littleaj
littleaj approved these changes Nov 9, 2018
View reviewed changes

@littleaj littleaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two main points:

  • There were some best practice comments which could lead to tests giving false positives after certain changes.
  • Consider adjusting your IDE settings to be less intrusive with optimizing imports and the like. There were quite a few changes which had nothing to do with this feature.

Comment thread ...er/src/test/java/com/microsoft/applicationinsights/internal/logger/FileLoggerOutputTest.java
Comment thread ...n/java/com/microsoft/applicationinsights/agent/internal/agent/AbstractHttpMethodVisitor.java
Comment thread agent/src/main/java/com/microsoft/applicationinsights/agent/internal/agent/CodeInjector.java
Comment thread .../java/com/microsoft/applicationinsights/agent/internal/agent/http/HttpClassDataProvider.java
Comment thread ...om/microsoft/applicationinsights/agent/internal/config/XmlAgentConfigurationBuilderTest.java Outdated
Comment thread ...crosoft/applicationinsights/autoconfigure/ApplicationInsightsTelemetryAutoConfiguration.java
@dhaval24 dhaval24 merged commit 6583478 into master Nov 9, 2018
@trask trask deleted the Agent/FileLogger branch September 21, 2020 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@littleaj littleaj littleaj approved these changes

@grlima grlima grlima approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dhaval24 dhaval24

Labels

Enhancement

Projects

None yet

Milestone

2.2.1

Development

Successfully merging this pull request may close these issues.

Update InternalAgentLogger with option to write to file

3 participants

@dhaval24 @littleaj @grlima