Skip to content

Conversation

@samgst-amazon
Copy link
Contributor

Implement polling and local caching logic for remote notification json file.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Description

Start an app level service that runs on startup that polls for new notifications once at startup and every 10 minutes after. Notifies listeners to process notifications when appropriate

Checklist

  • My code follows the code style of this project
  • I have added tests to cover my changes
  • A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
  • I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Nov 21, 2024

Qodana Community for JVM

5 new problems were found

Inspection name Severity Problems
Usage of redundant or deprecated syntax or deprecated symbols 🔶 Warning 3
Possibly blocking call in non-blocking context 🔶 Warning 1
Redundant modality modifier 🔶 Warning 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

@Service(Service.Level.APP)
class NotificationPollingServiceImpl :
NotificationPollingService,
PersistentStateComponent<NotificationPollingServiceImpl.State>,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably have a separate persistent state class that keeps track of the etag and get its instance here

override suspend fun execute(project: Project) {
val service = NotificationPollingService.getInstance()
ProcessNotificationsBase()
RunOnceUtil.runOnceForApp(this::class.qualifiedName.toString()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Will this startup activity anyway run only once?

Copy link
Contributor Author

@samgst-amazon samgst-amazon Nov 22, 2024

Choose a reason for hiding this comment

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

Switched to Atomic Boolean, but the same sort of logic applies. Because StartupActivity is obsolete in favor of ProjectActivity, we need to ensure we are only creating one instance of the polling service per IDE session. Otherwise, when a user opens multiple projects, it would start multiple instances of the polling service

Tried registering the service as an ApplicationInitializedListener, but wasn't able to get that working.


override suspend fun execute(project: Project) {
val service = NotificationPollingService.getInstance()
ProcessNotificationsBase()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ProcessNotificationsBase needs to initialize somehow (after the pollingservice init and before the start polling begins) so that it's observer is able to be notified on startup. I tried some other ways of implementing, but this is what I found to work

import software.aws.toolkits.jetbrains.core.DefaultRemoteResourceResolverProvider
import software.aws.toolkits.jetbrains.core.RemoteResourceResolverProvider
import software.aws.toolkits.telemetry.Component
import software.aws.toolkits.telemetry.ToolkitTelemetry

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

Remove deprecated symbol import
import software.aws.toolkits.jetbrains.core.DefaultRemoteResourceResolverProvider
import software.aws.toolkits.jetbrains.core.RemoteResourceResolverProvider
import software.aws.toolkits.telemetry.Component
import software.aws.toolkits.telemetry.ToolkitTelemetry

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'ToolkitTelemetry' is deprecated. Use type-safe metric builders
}

private fun emitFailureMetric(e: Exception?) {
ToolkitTelemetry.showNotification(

Check warning

Code scanning / QDJVMC

Usage of redundant or deprecated syntax or deprecated symbols Warning

'ToolkitTelemetry' is deprecated. Use type-safe metric builders
)

@Service(Service.Level.APP)
internal final class NotificationPollingService : Disposable {

Check warning

Code scanning / QDJVMC

Redundant modality modifier Warning

Redundant modality modifier
@samgst-amazon samgst-amazon marked this pull request as ready for review November 26, 2024 02:25
@samgst-amazon samgst-amazon requested a review from a team as a code owner November 26, 2024 02:25
Comment on lines 147 to 149
scope.launch {
delay(backoffDelay)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this will immediately return and continue the while loop

private val isFirstPoll = AtomicBoolean(true)
private val observers = mutableListOf<() -> Unit>()
private val alarm = AlarmFactory.getInstance().create(Alarm.ThreadToUse.POOLED_THREAD, this)
private val scope = CoroutineScope(getCoroutineBgContext())
Copy link
Contributor

Choose a reason for hiding this comment

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

CoroutineScope should always be injected by platform

resourceResolver.get()
.resolve(notificationsResource)
.toCompletableFuture()
.get()

Check warning

Code scanning / QDJVMC

Possibly blocking call in non-blocking context Warning

Possibly blocking call in non-blocking context could lead to thread starvation
* split notifications into separated lists.

* add persistent notification dismissal state logic

* boolean changes

* group persistant states

* comments

* Service initialized automatically

* isStartup global

* Deserialized notification schedule type

* tests

* persistent state syntax

* convert to light services

* Remove state from companion object

* detekt
* Main polling function that checks for updates and downloads if necessary
* Returns the parsed notifications if successful, null otherwise
*/
private fun pollForNotifications(): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

suspend

retryCount++
if (retryCount < MAX_RETRIES) {
val backoffDelay = RETRY_DELAY_MS * (1L shl (retryCount - 1))
scope.launch {
Copy link
Contributor

Choose a reason for hiding this comment

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

scope is not needed if function is suspend

override suspend fun execute(project: Project) {
if (initialized.compareAndSet(false, true)) {
val service = NotificationPollingService.getInstance()
ProcessNotificationsBase()
Copy link
Contributor

Choose a reason for hiding this comment

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

initialize through platform service system

if (current != null && !isExpired(current, resource)) {
LOG.debug { "Existing file ($current) for ${resource.name} is present and not expired - using it." }
return current
if (resource.name != "notifications.json") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rli This feels hacky, but its one way to get around this issue, the other way I think we could maybe do this is to just set the expiry time to a very short window so that the file is always seen as expired by the time polling makes this call to the endpoint

@samgst-amazon samgst-amazon merged commit 33bfa44 into feature/ideNotifs Nov 27, 2024
15 checks passed
@samgst-amazon samgst-amazon deleted the samgst/notificationPolling branch November 27, 2024 18:44
samgst-amazon added a commit that referenced this pull request Nov 29, 2024
* Add framework for processing notifications (#5112)

* Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (#5093)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* modified the base class

* modified test instance lifecycle

* Show toasts for notifications and notification banner on critical notifications(#5097)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Show notification banner

* feedback 1

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* feedback 1

* modified the base class

* merge conflicts resolved

* rearranged call site

* show notifications when panel is opened

* fixed tests

* detekt

* feedback

* convert panels into wrappers

* fixed test

* Adding update/restart action to notifications (#5136)

* Poll for new notifications (#5119)

* initial commit

* run on startup

* detekt

* move vals

* remote resource implementation

* comments

* detekt

* Validate file before saving

* cache path

* observer implementation

* deserialize notifs from file

* detekt

* remove unused interface

* internal class

* Fix observer

* etag singleton state component

* add telemetry

* atomicBoolean

* initialize once per IDE startup

* code scan

* Omit (Unit)

* specify etag storage location

* detekt

* fix detekt issues

* basic tests

* no star imports

* coroutine scope delay instead of thread.sleep

* feedback fixes

* test fix

* Application Exists for tests

* endpoint object

* detekt

* detekt fixes

* boolean flag

* boolean flag

* update tests

* move startup flag handling to processBase

* fix delay

* fix delay

* Notification dismissal state tracking  (#5129)

* split notifications into separated lists.

* add persistent notification dismissal state logic

* boolean changes

* group persistant states

* comments

* Service initialized automatically

* isStartup global

* Deserialized notification schedule type

* tests

* persistent state syntax

* convert to light services

* Remove state from companion object

* detekt

* endpoint as registryKey

* detekt

* fix startup issues

* Expiry issues

* Add logging info to IDE notification polling and processing (#5138)

* add logs for polling and processing notifs

* redundant

* finish log

* fix isFirstPoll not setting to false on first pass

---------

Co-authored-by: aws-toolkit-automation <[email protected]>
Co-authored-by: manodnyab <[email protected]>
Co-authored-by: Bryce Ito <[email protected]>
bryceitoc9 added a commit that referenced this pull request Nov 30, 2024
* Add framework for processing notifications (#5112)

* Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (#5093)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* modified the base class

* modified test instance lifecycle

* Show toasts for notifications and notification banner on critical notifications(#5097)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Show notification banner

* feedback 1

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* feedback 1

* modified the base class

* merge conflicts resolved

* rearranged call site

* show notifications when panel is opened

* fixed tests

* detekt

* feedback

* convert panels into wrappers

* fixed test

* Adding update/restart action to notifications (#5136)

* Poll for new notifications (#5119)

* initial commit

* run on startup

* detekt

* move vals

* remote resource implementation

* comments

* detekt

* Validate file before saving

* cache path

* observer implementation

* deserialize notifs from file

* detekt

* remove unused interface

* internal class

* Fix observer

* etag singleton state component

* add telemetry

* atomicBoolean

* initialize once per IDE startup

* code scan

* Omit (Unit)

* specify etag storage location

* detekt

* fix detekt issues

* basic tests

* no star imports

* coroutine scope delay instead of thread.sleep

* feedback fixes

* test fix

* Application Exists for tests

* endpoint object

* detekt

* detekt fixes

* boolean flag

* boolean flag

* update tests

* move startup flag handling to processBase

* fix delay

* fix delay

* Notification dismissal state tracking  (#5129)

* split notifications into separated lists.

* add persistent notification dismissal state logic

* boolean changes

* group persistant states

* comments

* Service initialized automatically

* isStartup global

* Deserialized notification schedule type

* tests

* persistent state syntax

* convert to light services

* Remove state from companion object

* detekt

* endpoint as registryKey

* detekt

* fix startup issues

* Expiry issues

* Add logging info to IDE notification polling and processing (#5138)

* add logs for polling and processing notifs

* redundant

* finish log

* show message instead of yesNoDialogue

* initialize ProcessNotificationsBase listener before Polling service

* fix isFirstPoll not setting to false on first pass

---------

Co-authored-by: aws-toolkit-automation <[email protected]>
Co-authored-by: manodnyab <[email protected]>
Co-authored-by: Bryce Ito <[email protected]>
karanA-aws pushed a commit to karanA-aws/aws-toolkit-jetbrains that referenced this pull request Jan 17, 2025
* Add framework for processing notifications (aws#5112)

* Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (aws#5093)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* modified the base class

* modified test instance lifecycle

* Show toasts for notifications and notification banner on critical notifications(aws#5097)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Show notification banner

* feedback 1

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* feedback 1

* modified the base class

* merge conflicts resolved

* rearranged call site

* show notifications when panel is opened

* fixed tests

* detekt

* feedback

* convert panels into wrappers

* fixed test

* Adding update/restart action to notifications (aws#5136)

* Poll for new notifications (aws#5119)

* initial commit

* run on startup

* detekt

* move vals

* remote resource implementation

* comments

* detekt

* Validate file before saving

* cache path

* observer implementation

* deserialize notifs from file

* detekt

* remove unused interface

* internal class

* Fix observer

* etag singleton state component

* add telemetry

* atomicBoolean

* initialize once per IDE startup

* code scan

* Omit (Unit)

* specify etag storage location

* detekt

* fix detekt issues

* basic tests

* no star imports

* coroutine scope delay instead of thread.sleep

* feedback fixes

* test fix

* Application Exists for tests

* endpoint object

* detekt

* detekt fixes

* boolean flag

* boolean flag

* update tests

* move startup flag handling to processBase

* fix delay

* fix delay

* Notification dismissal state tracking  (aws#5129)

* split notifications into separated lists.

* add persistent notification dismissal state logic

* boolean changes

* group persistant states

* comments

* Service initialized automatically

* isStartup global

* Deserialized notification schedule type

* tests

* persistent state syntax

* convert to light services

* Remove state from companion object

* detekt

* endpoint as registryKey

* detekt

* fix startup issues

* Expiry issues

* Add logging info to IDE notification polling and processing (aws#5138)

* add logs for polling and processing notifs

* redundant

* finish log

* fix isFirstPoll not setting to false on first pass

---------

Co-authored-by: aws-toolkit-automation <[email protected]>
Co-authored-by: manodnyab <[email protected]>
Co-authored-by: Bryce Ito <[email protected]>
karanA-aws pushed a commit to karanA-aws/aws-toolkit-jetbrains that referenced this pull request Jan 17, 2025
* Add framework for processing notifications (aws#5112)

* Add deserialization for notification messages retrieved from the notification file and criteria on whether it should be displayed (aws#5093)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* modified the base class

* modified test instance lifecycle

* Show toasts for notifications and notification banner on critical notifications(aws#5097)

* Display toast notifications with actions

* Condition matcher for displaying notifications

* Show notification banner

* feedback 1

* Modified deserialization cases and added tests

* not required file change

* feedback 1

* feedback 1

* modified the base class

* merge conflicts resolved

* rearranged call site

* show notifications when panel is opened

* fixed tests

* detekt

* feedback

* convert panels into wrappers

* fixed test

* Adding update/restart action to notifications (aws#5136)

* Poll for new notifications (aws#5119)

* initial commit

* run on startup

* detekt

* move vals

* remote resource implementation

* comments

* detekt

* Validate file before saving

* cache path

* observer implementation

* deserialize notifs from file

* detekt

* remove unused interface

* internal class

* Fix observer

* etag singleton state component

* add telemetry

* atomicBoolean

* initialize once per IDE startup

* code scan

* Omit (Unit)

* specify etag storage location

* detekt

* fix detekt issues

* basic tests

* no star imports

* coroutine scope delay instead of thread.sleep

* feedback fixes

* test fix

* Application Exists for tests

* endpoint object

* detekt

* detekt fixes

* boolean flag

* boolean flag

* update tests

* move startup flag handling to processBase

* fix delay

* fix delay

* Notification dismissal state tracking  (aws#5129)

* split notifications into separated lists.

* add persistent notification dismissal state logic

* boolean changes

* group persistant states

* comments

* Service initialized automatically

* isStartup global

* Deserialized notification schedule type

* tests

* persistent state syntax

* convert to light services

* Remove state from companion object

* detekt

* endpoint as registryKey

* detekt

* fix startup issues

* Expiry issues

* Add logging info to IDE notification polling and processing (aws#5138)

* add logs for polling and processing notifs

* redundant

* finish log

* show message instead of yesNoDialogue

* initialize ProcessNotificationsBase listener before Polling service

* fix isFirstPoll not setting to false on first pass

---------

Co-authored-by: aws-toolkit-automation <[email protected]>
Co-authored-by: manodnyab <[email protected]>
Co-authored-by: Bryce Ito <[email protected]>
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.

3 participants