-
Notifications
You must be signed in to change notification settings - Fork 66
Updated Export Frequency to 60 Seconds and Added Disk Export Parameter #958
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
base: main
Are you sure you want to change the base?
Conversation
…BufferingConfig config
Hi @Yadavanurag13 you will need to sign the CLA and get the build passing. Thanks! |
@breedx-splk I was trying to sign in as an individual contributor, but I got an error that this project is only for employer contributors is it so ? |
It's been a while since I signed it, but you should totally be able to sign as an individual contributor. https://opentelemetry.io/docs/contributing/prerequisites/#cla Please make sure that the git commits were made with the same email address that you are signing with (that sometimes trips people up). |
@@ -26,6 +27,7 @@ data class DiskBufferingConfig | |||
val maxFileAgeForReadMillis: Long = TimeUnit.HOURS.toMillis(DEFAULT_MAX_FILE_AGE_FOR_READ_MS), | |||
val maxCacheFileSize: Int = MAX_CACHE_FILE_SIZE, | |||
val debugEnabled: Boolean = false, | |||
val exportIntervalInSeconds: Long = DEFAULT_EXPORT_INTERVAL_IN_SECONDS, |
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.
should we use Duration
instead of Long
?
eg
opentelemetry-android/session/src/main/kotlin/io/opentelemetry/android/session/SessionConfig.kt
Lines 13 to 14 in c2ddbe0
val backgroundInactivityTimeout: Duration = 15.minutes, | |
val maxLifetime: Duration = 4.hours, |
|
||
override fun enable() { | ||
if (!enabled.getAndSet(true)) { | ||
periodicWorkService.enqueue(exportScheduler) | ||
if (scheduler == null) { |
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 check was thread safe with the AtomicBoolean
and now its not anymore, is there a reason to remove the atomic operation?
@Yadavanurag13 can you address the few comments above please? Thanks again! |
@breedx-splk Sure !! |
@Yadavanurag13, there are still some open comments, and CI is broken. Do you intend to come back to it? |
Sorry @marandaneto. Actually I busy with my personal task. Right now I don't have much time to spent. |
This PR closes #947
I have updated the export frequency to 60 sec and added the parameter in the config.
I'm raising this pr. I'm new to this repo so, In case there any review comments pls let me know.