-
Notifications
You must be signed in to change notification settings - Fork 199
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
Reset CDS Profile fetch counters + Introduce generic PeriodTaskExecutor #901
Conversation
...rc/main/java/com/microsoft/applicationinsights/internal/profile/CdsProfileFetcherPolicy.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/microsoft/applicationinsights/internal/profile/CdsProfileFetcherPolicy.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/microsoft/applicationinsights/internal/profile/CdsProfileFetcherPolicy.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/microsoft/applicationinsights/internal/profile/CdsProfileFetcherPolicy.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/microsoft/applicationinsights/internal/profile/CdsProfileFetcherPolicy.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/microsoft/applicationinsights/internal/profile/CdsProfileFetcherPolicy.java
Outdated
Show resolved
Hide resolved
.../main/java/com/microsoft/applicationinsights/web/internal/correlation/CdsProfileFetcher.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my comments: The major things are the field visibility and log message.
Also, let's discuss the necessity of the resetService.
test assert suggestion is minor.
.../java/com/microsoft/applicationinsights/web/internal/correlation/CdsProfileFetcherTests.java
Outdated
Show resolved
Hide resolved
.../main/java/com/microsoft/applicationinsights/web/internal/correlation/CdsProfileFetcher.java
Outdated
Show resolved
Hide resolved
resetService = Executors.newSingleThreadScheduledExecutor( | ||
ThreadPoolUtils.createDaemonThreadFactory(CdsProfileFetcher.class, "CdsProfilePurgeService")); | ||
long cachePurgeInterval = policyConfiguration.getResetPeriodInMinutes(); | ||
resetService.scheduleAtFixedRate(new CachePurgingRunnable(), cachePurgeInterval, cachePurgeInterval, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhaval24 I wonder if we need an additional executor for purging the cache. If fetchProfile
is used frequently, you could store the last time it was called and purge the cache after elapsed time is over the threshold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@littleaj there is a slight confusion due to naming. This should more be resetInterval. I have pushed the changes. I do not purge cache and that is not needed. We only reset the counter. If cache part of InstrumentationKeyResolver
already has the appId resolved there is no need to retry.
Regarding your comment on do we need additional service here. I would prefer to have seperate service as it adds clean seperation of what task is designated to which service.
...rc/main/java/com/microsoft/applicationinsights/internal/profile/CdsProfileFetcherPolicy.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/microsoft/applicationinsights/internal/profile/CdsRetryPolicy.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/microsoft/applicationinsights/internal/profile/CdsRetryPolicy.java
Outdated
Show resolved
Hide resolved
/** | ||
* Cached instance to be reused across SDK for CDS Profile fetch calls. | ||
*/ | ||
INSTANCE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good learning to me, never heard of it before :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the use of enum? :)
core/src/main/java/com/microsoft/applicationinsights/internal/util/TimerTaskUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/microsoft/applicationinsights/internal/profile/CdsRetryPolicy.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an offline conversation with @reyang he outlined additional concerns he has.
- Current design - has no ability to kill/stop a task. The util class should have it.
- The Map used to store taskId and service can be a source of memory leak as it stays for lifecycle of app.
To address @reyang comments. I will change the design to:
- Add cancellation capability for a task.
- Instead of creating a ExecutorService for each task, have a common service with 2 threads (can be tweaked in future as per need).
- Map stores string and future (which can be cancelled). Once a future is cancelled, it would be removed from the map.
@reyang I have redesigned the Additionally, I have also tried to resolve your previous comments. |
Currently CDS Profiler fetcher retries instantly 3 times. If the retry fails, it never retries for the entire application lifecycle. This leads to broken app-map experience as correlation needs ikey-appId resolution.
This PR fixes this by resetting the retry counter in
CdsProfileFetcher.java
every 4 hours by default. This configuration can be over-riden.This PR also introduces a Generic Abstraction for creating, managing and running PeriodicTask called
PeriodicTaskManager
In upcoming PR I will make this configurable via XML and Springboot starter.