care partner alerts #715

ewollesen · 2024-05-06T15:55:54Z

This used to be a series of PRs, but that didn't really work out. They're all collapsed into this one.

Shouldn't be merged until tidepool-org/go-common#71 is merged, then this should have it's go-common bumped.

ewollesen · 2024-07-11T19:11:07Z

To use this in QA, it must be paired with tidepool-org/hydrophone#145 and tidepool-org/go-common#71

toddkazakov

Looks good overall, but the retry mechanism which is implemented here doesn't satisfy the latency requirements. The current implementation is ok for internal usage, but it's not production ready. This could be handled in a separate PR if this makes the development and QA process easier.

auth/store/mongo/device_tokens_repository.go

auth/store/test/device_token_repository.go

data/events/alerts.go

toddkazakov · 2024-07-12T12:56:39Z

data/events/events.go

+	}
+	handler := asyncevents.NewSaramaConsumerGroupHandler(&asyncevents.NTimesRetryingConsumer{
+		Consumer: r.Config.MessageConsumer,
+		Delay:    CappedExponentialBinaryDelay(AlertsEventRetryDelayMaximum),


I don't think this is a suitable retry strategy given the latency requirements for this service. Kafka's consumer group concurrency is limited to the number of partitions of the topic. This number cannot be very high because Kafka's memory consumption grows linearly with the number of partitions. From this follows that the number of partitions is much lower than the number of users we will have and the data of multiple users will end up in the same partition. A failure to evaluate a single user's alerts for one minute as currently set by the CappedExponentialBinaryDelay will introduce at least a minute delay to all of the users sharing the same partition, because messages in a single partition are processed serially.

Alert notifications should be near real-time - up to 10 seconds latency is acceptable. I think the solution proposed in this design document is how this should be handled. Other solutions which satisfy the requirements are welcome.

This will require some more in-depth thought on my part... Will do.

Yeah, I think you're right, let's get this review merged, and I'll work on getting a multiple topic solution set up. Given the flexibility we have now, it shouldn't be too bad.

I have the multi-tier retry in the eric-alerts-multi-topic-retry branch.

This should be implemented in this branch now.

data/events/events.go

data/service/service/standard.go

ewollesen · 2024-10-07T19:28:03Z

@toddkazakov I just removed two config env vars. I believe we talked about that before, but it slipped my mind until I was reviewing the helm chart changes today, where they came up again.

So the re-review here is just around the config parsing, in the most recent commit of the PR, nothing else is changed. [UPDATE] this comment is outdated.

BACK-2499 BACK-2559

Requested in code review. #715 (comment) BACK-2499 BACK-2559

BACK-2499 BACK-2559

BACK-2559

darinkrauss · 2025-02-13T19:47:37Z

task/queue/queue.go

+		if tsk.AvailableTime == nil {
+			tsk.AvailableTime = pointer.FromAny(time.Now())
+		} else if time.Now().After(*tsk.AvailableTime) {
 			tsk.AppendError(errors.New("pending task requires future available time"))
 			tsk.SetFailed()
 		}


Actually, after thinking a bit more on this, let's change this to:

Suggested change

if tsk.AvailableTime == nil {

tsk.AvailableTime = pointer.FromAny(time.Now())

} else if time.Now().After(*tsk.AvailableTime) {

tsk.AppendError(errors.New("pending task requires future available time"))

tsk.SetFailed()

}

if now := time.Now(); tsk.AvailableTime == nil || tsk.AvailableTime.Before(now) {

tsk.AvailableTime = pointer.FromAny(now)

}

This should fully resolve any edge cases (due to timing, etc.) plus allow for nil to just mean "ASAP".

darinkrauss · 2025-02-13T19:52:06Z

data/events/alerts.go

@@ -0,0 +1,233 @@
+package events


Is this part of the formal (public) Tidepool data model or is it just an internal-only (never to be exposed to the user)? If public, then I'd like to review and comment, but it'll need to wait a bit until I have more time.

Is this part of the formal (public) Tidepool data model or is it just an internal-only (never to be exposed to the user)? If public, then I'd like to review and comment, but it'll need to wait a bit until I have more time.

Users would never need to interact with this directly, no. The events here are the mechanism by which we trigger and send alerts to care partners.

This code is being "shelved" due to our recent partner's contract cancellation.

Does "shelved" mean merge into master? If so, we need to be sure this code isn't intentionally or unintentionally used.

@darinkrauss the alert configuration objects in alerts/config.go are part of the public API (not the data model, specifically). AFAIK, there aren't a new data types introduced as part of this work.

darinkrauss · 2025-02-13T19:53:49Z

task/service/service/service.go

+
+	platformConfig := platform.NewConfig()
+	platformConfig.UserAgent = s.UserAgent()
+	reporter := s.ConfigReporter().WithScopes("data", "client")


Why is this using "data" client, shouldn't this be "alert(s)" client?

Why is this using "data" client, shouldn't this be "alert(s)" client?

The alerts client uses endpoints that are exposed by the data service, just like the data client. I saw that I could re-use this value and not have to make additional changes to other repos. Perhaps it would have been better if this had been the data service config, because then any client that needed to talk to the data service could use it with this kind of confusion.

Duplicating the value is a thing we can do, but that too might cause some confusion then there'd be two different clients that use the same values and both would need to be updated in the event of a change there, and it could be confusing as to why they're not in the same client in the first place. But it was decided a good while ago that we didn't want a separate alerts service.

I see downsides either way, so made a judgment call. Would a comment be of use here?

darinkrauss · 2025-02-13T19:56:40Z

task/service/service/service.go

+func (s *Service) initializePusher() error {
+	var err error
+
+	apns2Config := &struct {


Typically we define the config struct alongside the object is is configuring (APNSPusher, in this case). It allows greatly isolation of knowledge. Also, my personal preference is that we keep using ConfigReporter rather than adding in yet another configuration mechanism.

I would like us to move away from config reporter. With many seemingly similar components which use generic names (services, clients, service clients, standard clients, etc) it's absolutely impossible to determine where an env variable is used and how the configuration is loaded.

I agree, that makes complete sense to me. I agree that ConfigReporter isn't completely obvious and moving to a more direct mechanism for configuration is a good idea.

I think, however, we should have a specific, agreed-upon plan for doing so. I'd rather not have multiple alternatives to ConfigReporter in the code, but one agreed upon solution going forward. Plus, I think we may be able to update the various configs with specific struct fields and apply a surgical change to ConfigReporter that would prevent a large change to remove ConfigReporter (at least in the short-term).

It gets worse. While envconfig is a nice package, it doesn't work the way we need it to. The problem comes when you start embedding structs and needing to alter the "prefix" of the environment vars that get looked up based on that embedding.

For example auth.Config embeds platform.Config which in turn embeds client.Config.

That client.Config has a field called Address. The auth client wants the value found at TIDEPOOL_AUTH_CLIENT_ADDRESS, however the data client wants the value at TIDEPOOL_DATA_SERVICE_ADDRESS. But there's no way to communicate that from the auth client's auth.Config or the data client's platform.Config (data client doesn't have it's own data.Config).

So if we were to try to use only envconfig, we're now faced with a prefix problem. That is, it isn't possible with envconfig to annotate an embedded struct such that the value in the annotation is appended to the existing prefix. Even if that were possible, we wouldn't know what that prefix should be.

To make this concrete, here are the same three structs, stripped down a little and re-annotated (bluntly) for use with envconfig:

// client.Config type Config struct { Address string `envconfig:"ADDRESS"` // There's no way to know what prefix to use yet, // this struct is only used after being embedded in // another struct. That is, we want this to be // TIDEPOOL_AUTH_CLIENT_ADDRESS. If that prefix // could be passed down, we might be onto // something. UserAgent string `envconfig:"USER_AGENT"` // Assuming we could pass the prefix down to // Address above, we _don't_ want a prefix // here! That said, we've never changed the // user agent that I'm aware of, so maybe this // could be removed as a config option? But // that's not the point. The point is: while // some embedded struct fields want to inherit // a prefix, others don't. So not only would // we have to enhance envconfig with a way to // inherit prefixes, we'd have to add a tag to // bypass prefix inheritance. } // platform.Config type Config struct { *client.Config // There's no way to know what the prefix should be here, because // platform.Config is always used from within another client's Config // (like auth.Config below), so even if we could add a prefix here (which // we can't), we wouldn't know what it should be! Perhaps we wouldn't // need/want a prefix here, but rather we'd just want to pass any given // prefix along. ServiceSecret string `envconfig:"SERVICE_SECRET"` // This value too, needs a prefix, but // platform doesn't know what that // value should be. It's only the // struct that embeds this one that // knows what this value should be // annotated with. } // auth.Config type Config struct { *platform.Config `envconfig:"AUTH_CLIENT"` // Having this appended to the prefix would // be a start, but still not enough, because // we'd need the prefix to also be passed // down to the platform.Config, and from // there again down to the Address field // that's defined in client.Config. *ExternalConfig `envconfig:"AUTH_CLIENT"` }

So... what's the solution? I'm always struck by how little actual code is in each of the various Config's. So my suggestion is to get rid of client.Config and platform.Config. Take the fields from them and copy them into the auth.Config, and make a data.Config that has them, and then use envconfig without any struct embedding. It would mean a little bit of duplication, but we gain simpler configuration. The only methods on client.Config and platform.Config relate to either validation or implementing a bridge interface between envconfig and ConfigReporter. The validations will be easy to copy over, and the bridge interface will be removed.

The duplication is unfortunate, but I think acceptable. It's not an area of high churn and it's all code that's easy to understand.

Update: Thinking about this a little more, we don't necessarily need to know what the various prefixes should be at compile-time, but rather at runtime, when envconfig.Process is called, and that is doable... We still wouldn't be able to grep for the full name of a config variable that way, but I'm pretty sure we could use just envconfig. I'll do a test.

While envconfig is a nice package, it doesn't work the way we need it to. The problem comes when you start embedding structs and needing to alter the "prefix" of the environment vars that get looked up based on that embedding.

I would argue that this is something that we don't want and is an anti-pattern. You're familiar with the code above, but what you described is not obvious by just reading it. Environment variables are "flat" - they don't represent a tree structures - trying to make them fit one is a non-starter. Our config structs should be flat - if there is behaviour that is common to those config structs, then this behaviour should be put behind an interface.

I absolutely agree that duplication is perfectly fine as it simplifies the code a whole lot. Consider the following example that I think is much easier to understand.

type UserAgentConfig struct { UserAgent string `envconfig:"TIDEPOOL_SERVICE_USER_AGENT"` } type TaskServiceCredentials struct { ClientID string `envconfig:"TIDEPOOL_TASK_SERVICE_CLIENT_ID"` ClientSecret string `envconfig:"TIDEPOOL_TASK_SERVICE_CLIENT_SECRET"` } // Implement Credentials interfaces func (b TaskServiceCredentials) GetClientID() string { return b.ClientID } func (b TaskServiceCredentials) GetClientSecret() string { return b.ClientSecret } type BlobServiceCredentials struct { ClientID string `envconfig:"TIDEPOOL_BLOB_SERVICE_CLIENT_ID"` ClientSecret string `envconfig:"TIDEPOOL_BLOB_SERVICE_CLIENT_SECRET"` } // Implement Credentials interfaces func (b BlobServiceCredentials) GetClientID() string { return b.ClientID } func (b BlobServiceCredentials) GetClientSecret() string { return b.ClientSecret } type OAuthCredentials interface { GetClientID() string ClientClientSecret() string } type AuthenticationClientConfig struct { Address string `envconfig:"TIDEPOOL_AUTH_SERVICE_ADDRESS"` } type DataClientConfig struct { Address string `envconfig:"TIDEPOOL_DATA_SERVICE_ADDRESS"` } func InitTaskService() error { dataClientConfig := DataClientConfig{} if err := envconfig.Process("", &dataClientConfig); err != nil { return err } authClientConfig := AuthenticationClientConfig{} if err := envconfig.Process("", &authClientConfig); err != nil { return err } creds := TaskServiceCredentials{} if err := envconfig.Process("", &creds); err != nil { return err } ... dataClient := NewDataClient(dataClientConfig, authClientConfig, userAgentConfig, creds) ... } func NewDataClient(dataClientConfig DataClientConfig, authClientConfig AuthenticationClientConfig, userAgentConfig UserAgentConfig, credentials OAuthCredentials) DataClient { // auth client for obtaining and refreshing service tokens authClient := NewExternalAuthClient(authClientConfig, credentials) ... } func NewExternalAuthClient(authClientConfig AuthenticationClientConfig, credentials Credentials) { // configure the OAuth client here }

Let's discuss this in a meeting.

I agree this is out-of-scope for this ticket, particularly if it is being "shelved" (though not sure what that means).

With that said, I do think we should have a discussion. I do like the use of interfaces for configuration. I'd like to consider https://github.com/caarlos0/env as an alternative. I used it in a past project, it is seeing regular support, and does handle prefixes.

I think part of the problem with our configuration isn't so much that the prefixes are a problem, but rather that the configuration is all over the place. What I'd like to see (and what has worked very well for me in the past) is a single global config for the service that has all of the configuration required for the service. It can use sub-structs with env prefixes, but it is very easy to see (and validate/test) that all required configuration is available immediately upon service startup.

In any case, a meeting at some point would be good.

I'd like to consider https://github.com/caarlos0/env as an alternative.

This one would work too. I proposed envconfig, because it's already used in most of the services, including platform.

I think part of the problem with our configuration isn't so much that the prefixes are a problem, but rather that the configuration is all over the place.

I have lost numerous hours trying to piece together whether a value that's being configured in the helm chart is interpreted correctly not only where it's loaded, but where it's used too - both in development and production environments when debugging issues. Since configuration is passed from kubernetes to the services as env variables, it should easy to find the configuration values both in the helm chart and in the code. The only way to achieve this that I see is to use full environment variable names so they are searchable in both repositories.

What I'd like to see (and what has worked very well for me in the past) is a single global config for the service that has all of the configuration required for the service.

I see the value in having the configuration in a single place, but having it split in modules has never been an issue for me. The two problems that deal with frequently are:

I need to add a configuration value in the helm chart after I made a code change. What is the name of the environment variable that I should in the helm chart so it's loaded correctly (going from code to config)

The service I am working on is experiencing an issue and it acts as if the environment is not configured correctly. The environment variables in the container look correct. I need to find the exact place in the code where a given environment variable is being used (going from config to code).

I have lost numerous hours trying to piece together whether a value that's being configured in the helm chart is interpreted correctly not only where it's loaded, but where it's used too

and

but having it split in modules has never been an issue for me

Seem to be in conflict with each other?

I'm firmly in the camp of the first quoted segment, and fully agree with Darin's desire to have all config loaded as close to "main" as possible, then a simple config struct passed around from there as/if necessary. At least having all env vars loaded there.

Seem to be in conflict with each other?

Only in the context of platform. Take the clinic service for example - https://github.com/search?q=repo%3Atidepool-org%2Fclinic%20envconfig&type=code. Configuration is loaded in each module, however it's pretty clear the source of the configuration values.

I'm firmly in the camp of the first quoted segment, and fully agree with Darin's desire to have all config loaded as close to "main" as possible, then a simple config struct passed around from there as/if necessary.

I am not convinced this is such a good idea, because it increases coupling and reminds me of the God Object anti-pattern. It's something I can live with as longs as I can grep environment variable names.

Oh I definitely don't mean we should have a god object no. This is a dumb struct that does nothing but binds the necessary config together for any one given service.

Previously, when completing a task, an available time of nil would cause the task to be marked as failed. Now, when a task completes and has available time of nil, time.Now() is substituted, which should cause the task to be run again ASAP. In addition, if the available time is in the past, it is substituted with time.Now(), so that it will run again ASAP. This supports the care partner no communication check, which wants to run 1x/second, but as that's not available with the task service (the smallest interval is 5 seconds), setting the value to 1 second intervals will run the task on each task service iteration. BACK-2559

toddkazakov · 2025-02-19T12:11:02Z

data/store/mongo/mongo_recorder.go

+
+// lastCommunicationsRepo implements LastCommunicationsRepository, writing data to a
+// MongoDB collection.
+type lastCommunicationsRepo structuredmongo.Repository


Please update the file name to reflect the name of the repository

toddkazakov · 2025-02-19T12:11:54Z

data/store/mongo/mongo_recorder.go

+// MongoDB collection.
+type lastCommunicationsRepo structuredmongo.Repository
+
+func (r *lastCommunicationsRepo) RecordReceivedDeviceData(ctx context.Context,


Update all pointer receivers so they are the same and match the name of the type.

They do..? r is for repo in this case. I've generally named my receivers after the noun in the name, not the adjectives in front of it. That generally makes more sense to me, as I think of this struct as a "repo" first and foremost.

Oooh, I see there was one still using d, maybe from a previous name that I missed. Should be fixed now.

Oh, how I wish Google hadn't screwed this up. It would have been so straightforward (and inline with keeping things clean and simple) to just use self or enforce some sort of standard (e.g. always lowercased first letter of struct name). This is one of those things that grates on my nerves. I know silly, but...

This is probably a little nitpicky, but I find it unconventional and confusing. In this example (LastCommunicationsRepo), the adjective is "last" and the first noun is "communications". In AlertsDataRepository the first word is either a plural noun Alerts or a singular possessive form, but the name of the pointer receiver is d. I find the use of r in LastCommunicationsRepo and d for AlertsDataRepository an arbitrary choice.

toddkazakov · 2025-02-19T12:13:19Z

data/store/mongo/mongo_recorder.go

+}
+
+func (d *lastCommunicationsRepo) OverdueCommunications(ctx context.Context) ([]alerts.LastCommunication, error) {
+	start := time.Now().Add(-alerts.MinimumNoCommunicationDelay)


Shouldn't the constant be defined in this package instead?

Huh, yeah... This should be used in alerts to validate the minimum delay setting on the alerts.Config. But it appears not to be at the moment. I'll fix that.

Update: to be clear, I think it makes more sense in alerts, as that's where it's used for validation/instantiation stuff, but if you'd prefer it lived here, I can move it.

It's not a pattern that I'm used to seeing and looked strange at first sight. I'll leave it up to you to decide what's best.

toddkazakov · 2025-02-19T12:19:04Z

data/store/mongo/mongo_alerts.go

+	}
+}
+
+type alertsDataRepo structuredmongo.Repository


It's not obvious that this uses the device data collection. It's odd that the functions below are not in mongo.go.

I'm not sure what would make things obvious to you, but I'll move them to the mongo.go and take a stab at it. Having a suggestion from you would help though, as I don't have any issue with the current name.

Why is this a separate repository if it all it does is query the device data collection? It's not a distinct repository where alertable data is stored. My suggestion would be to move the implementation to the main device data repo.

Hmmm... Not sure. I think I did it originally to make testing easier (smaller interface), but now that I'm revisiting it, I think I can see the existing pattern and yeah, I think this can get merged back into the existing DataRepository.

Thank you for pointing it out.

toddkazakov · 2025-02-19T12:23:04Z

data/service/service/standard.go

+
+type alertsEventsHandlerConfig struct {
+	*platform.Config
+	RetryDelaysConfig string `envconfig:"TIDEPOOL_DATA_SERVICE_ALERTS_RETRY_DELAYS" default:"1s"`


Is this still used?

No. Good catch, thanks.

toddkazakov · 2025-02-19T12:30:50Z

data/service/api/v1/alerts.go

@@ -24,6 +24,8 @@ func AlertsRoutes() []service.Route {
 		service.Get("/v1/users/:userId/followers/:followerUserId/alerts", GetAlert, api.RequireAuth),
 		service.Post("/v1/users/:userId/followers/:followerUserId/alerts", UpsertAlert, api.RequireAuth),
 		service.Delete("/v1/users/:userId/followers/:followerUserId/alerts", DeleteAlert, api.RequireAuth),
+		service.Get("/v1/users/:userId/followers/alerts", ListAlerts, api.RequireServer),
+		service.Get("/v1/users/overdue_communications", ListOverdueCommunications, api.RequireServer),


/v1/users/overdue_communications matches the path /v1/users/:userId. It should be changed so it reflects the resource name - e.g. /v1/overdue_communications.

toddkazakov · 2025-02-19T12:34:09Z

data/store/mongo/mongo_recorder.go

+}
+
+func (d *recorderRepo) UsersWithoutCommunication(ctx context.Context) ([]alerts.LastCommunication, error) {
+	start := time.Now().Add(-5 * time.Minute)


UsersWithoutCommunication doesn't actually reflect what the implementation does - it only returns users which didn't have communication in the last 5 minutes. UsersWithoutCommunication(300 * time.Second) on the other hand reflects this. I see that you renamed this to OverdueCommunications which is better.

alerts/config.go

toddkazakov · 2025-02-19T12:48:07Z

data/events/alerts.go

@@ -0,0 +1,233 @@
+package events


@darinkrauss the alert configuration objects in alerts/config.go are part of the public API (not the data model, specifically). AFAIK, there aren't a new data types introduced as part of this work.

toddkazakov · 2025-02-19T12:49:41Z

data/service/service/standard.go

+func (s *Standard) initializePusher() error {
+	var err error
+
+	apns2Config := &struct {


I think it makes sense to define this struct where the config values are used as Darin suggested.

This struct isn't used anywhere else. You know that, because it's defined inline. There is nowhere else for it to be defined that makes any sense, as this is the one and only place it's used.

It's used only to get these env vars. I can switch to os.Getenv if you prefer. But defining the struct somewhere else is only going to make things more confusing.

This struct isn't used anywhere else, but the config values are used in the push package which provides an implementation of a push notification service. The push notification service can be reused - it's not a data service specific implementation. What if we wanted to use push notification in the prescription service (real example that has been discussed in the past)? We would need to define this same struct in the prescription service and duplicate the code for loading the env variables and instantiation of the pusher. This is how you end up with hundreds of lines of duplicated code that we have to maintain, e.g.:

https://github.com/tidepool-org/platform/blob/master/blob/service/service.go

https://github.com/tidepool-org/platform/blob/master/auth/service/service/service.go

https://github.com/tidepool-org/platform/blob/master/data/service/service/standard.go

If the pusher is used by multiple services and you make a change to the configuration parameters or how the configuration is loaded, then you would need to make the exact same change in multiple places. My suggestion would be to define a named struct in the push package and instantiate it with a single call. I think we both agreed during our huddle on Wednesday that configuration should be defined where it's used but loaded as close to the service entry point as possible.

I would like to give you an example of an alternative design. Consider the clinic client module. Both the configuration struct and initialization logic are contained in the clinic module (package). Using the clinic client in services which use fx for dependency injection is as easy as including the constructor in their dependecy list. Services which manually instantiate their dependencies (e.g. the task service, need to call a single function to obtain a fully configured and ready to use clinic client.

We would need to define this same struct in the prescription service and duplicate the code for loading the env variables and instantiation of the pusher.

Why would you duplicate the loading code? I would extract the struct out then, if I needed it in multiple places? Keep it simple / Laziness is a virtue / You aren't going to need it.

then you would need to make the exact same change in multiple places

Only if you duplicated all of that code instead of extracting the struct out and reusing it.

I will analyze your example and attempt to mimic it for the pusher.

Well... I get to eat my own words.

I did duplicate that anonymous struct! Shame on me!

I did it because the environment variables loaded were different for each service, but they don't need to be, so I can collapse them down, there'll be a change in development coming.

So yeah, I used two separate anonymous structs to load from differently named env vars. In this case, we don't want separate env vars, so I'm collapsing them down into a single struct in the alerts package. It's not in push, because the config is specific to care partner alerts. Other packages will likely want to use other values.

The downside of this is that now alerts needs to know about envconfig, which I think is unfortunate. I feel like the "main" package or maybe for platform the "service" package should be the one that knows about config values and how to load them. Oh well. If we re-arrange our config value stuff, we can look at moving things around then.

tidepool-org/development#316

https://github.com/tidepool-org/cluster-qa2/pull/1563

https://github.com/tidepool-org/cluster-integration/pull/497

https://github.com/tidepool-org/cluster-production/pull/711

I'm not deploying these until Monday, just to be safer. I don't need to ruin anyone's weekend.

ewollesen · 2025-02-19T15:16:58Z

Not sure why I can't reply inline, but...

UsersWithoutCommunication doesn't actually reflect what the implementation does - it only returns users which didn't have communication in the last 5 minutes. UsersWithoutCommunication(300 * time.Second) on the other hand reflects this. I see that you renamed this to OverdueCommunications which is better.

I disagree that it's useful to know that, given that the value doesn't (and shouldn't) change at runtime, this adds complexity without purpose.

But it's not a big deal, so I'll go ahead and make the change.

ewollesen · 2025-02-19T15:32:24Z

But it's not a big deal, so I'll go ahead and make the change.

No I won't. We discussed this via Slack and decided it wasn't necessary. The name change is enough.

…m duration BACK-2559

…ommunications BACK-2559

This need for this struct went away when retry delays were removed. BACK-2559

The minimum (and default value) is 5m. Validate that configs specify either 0, or a value in the range from 5m-6h. BACK-2559

This minimizes the Kafka load for non production environments. BACK-2559

BACK-2559

As Todd pointed out in code review, there's no need for this to be separate from the existing DataRepository, as it only queries the deviceData collection. BACK-2559

BACK-2559

The tasks service and data service can both push alerts for care partners. This change consolidates that configuration into one set of environment variables loaded via the alerts package. I wish that alerts didn't need to know about envconfig, but at least for the moment it's the only way to consolidate the information about the configuration into a single re-usable struct. Naming of the pusher in both services is prefixed with "alerts" to communicate that this pusher is configured for care partner alerts. BACK-2559

ewollesen requested a review from toddkazakov May 6, 2024 15:55

ewollesen mentioned this pull request May 6, 2024

lift Repeat out of the base alert config #716

Closed

ewollesen removed the request for review from toddkazakov May 8, 2024 19:59

ewollesen force-pushed the eric-cpa-alerts branch from 374bae2 to 2deda23 Compare June 6, 2024 17:52

ewollesen force-pushed the eric-cpa-alerts branch 2 times, most recently from 8549c33 to 8367902 Compare June 24, 2024 19:09

ewollesen force-pushed the eric-cpa-alerts branch from 8367902 to 2ea9686 Compare July 9, 2024 22:43

ewollesen changed the title ~~adds List and Get methods to alerts client~~ minimal implementation of care partner alerts Jul 9, 2024

ewollesen requested a review from toddkazakov July 9, 2024 22:50

ewollesen force-pushed the eric-cpa-alerts branch from 2ea9686 to 7246848 Compare July 10, 2024 17:31

ewollesen force-pushed the eric-cpa-alerts branch 2 times, most recently from c50d589 to 986106b Compare July 11, 2024 22:55

ewollesen mentioned this pull request Jul 11, 2024

add evaluation of not looping alerts #751

Closed

toddkazakov reviewed Jul 12, 2024

View reviewed changes

ewollesen force-pushed the eric-cpa-alerts branch 2 times, most recently from c08e1fc to 967c617 Compare July 19, 2024 16:02

ewollesen requested a review from toddkazakov July 29, 2024 22:41

ewollesen force-pushed the eric-cpa-alerts branch from 967c617 to cd42b19 Compare July 29, 2024 22:44

ewollesen force-pushed the eric-cpa-alerts branch 2 times, most recently from 9432468 to eaa652e Compare September 17, 2024 20:07

toddkazakov previously approved these changes Sep 18, 2024

View reviewed changes

ewollesen dismissed toddkazakov’s stale review via ee5da4a October 2, 2024 17:56

ewollesen force-pushed the eric-cpa-alerts branch from d6449a1 to ee5da4a Compare October 2, 2024 17:56

toddkazakov previously approved these changes Oct 5, 2024

View reviewed changes

ewollesen dismissed toddkazakov’s stale review via 8ae1dc8 October 7, 2024 19:25

ewollesen requested a review from toddkazakov October 7, 2024 19:26

ewollesen force-pushed the eric-cpa-alerts branch from 8ae1dc8 to 28fdf06 Compare October 31, 2024 15:40

ewollesen force-pushed the eric-cpa-alerts branch from 28fdf06 to b9767dc Compare December 11, 2024 19:54

ewollesen added 5 commits February 10, 2025 16:51

remove un-needed comment

55b9150

BACK-2499 BACK-2559

move care partner task definition to alerts package

2aa72c5

BACK-2499 BACK-2559

make GetRunnerDeadline() use a multiple of GetRunnerDurationMaximum()

1140500

Requested in code review. #715 (comment) BACK-2499 BACK-2559

replace magic number with named constant

0586c46

BACK-2499 BACK-2559

adds environment-based config for alerts retry delays

8f3d3ab

BACK-2559

ewollesen force-pushed the eric-cpa-alerts branch from b5dd269 to 0da82d6 Compare February 12, 2025 23:23

ewollesen requested review from darinkrauss and toddkazakov February 12, 2025 23:25

darinkrauss reviewed Feb 13, 2025

View reviewed changes

ewollesen force-pushed the eric-cpa-alerts branch from 0da82d6 to 557638e Compare February 18, 2025 14:57

ewollesen force-pushed the eric-cpa-alerts branch from 557638e to 7a55626 Compare February 18, 2025 15:35

toddkazakov requested changes Feb 19, 2025

View reviewed changes

ewollesen added 6 commits February 20, 2025 07:37

the alerts task's GetRunnerTimeout should be a multiple of its maximu…

5ddc157

…m duration BACK-2559

rename API endpoint /v1/users/overdue_communications => /v1/overdue_c…

c405411

…ommunications BACK-2559

remove unused config struct

56a00ad

This need for this struct went away when retry delays were removed. BACK-2559

validate overdue communication minimum value

d510d05

The minimum (and default value) is 5m. Validate that configs specify either 0, or a value in the range from 5m-6h. BACK-2559

assume no alerts retry topics by default

a9fe904

This minimizes the Kafka load for non production environments. BACK-2559

fix a receiver name to match others defined on the struct

5cc265c

BACK-2559

ewollesen force-pushed the eric-cpa-alerts branch from 23349a1 to 292bc2e Compare February 20, 2025 14:37

ewollesen requested a review from toddkazakov February 20, 2025 14:55

ewollesen added 3 commits February 21, 2025 09:28

merge alertsDataRepo into DataRepository

e2da7cd

As Todd pointed out in code review, there's no need for this to be separate from the existing DataRepository, as it only queries the deviceData collection. BACK-2559

rename method receiver

516750a

BACK-2559

ewollesen force-pushed the eric-cpa-alerts branch from 292bc2e to a1b7712 Compare February 28, 2025 20:21

care partner alerts #715

Are you sure you want to change the base?

care partner alerts #715

Conversation

ewollesen commented May 6, 2024 • edited Loading

ewollesen commented Jul 11, 2024 • edited Loading

toddkazakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen commented Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

toddkazakov Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddkazakov Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewollesen Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

ewollesen commented Feb 19, 2025

ewollesen commented Feb 19, 2025

ewollesen commented May 6, 2024 •

edited

Loading

ewollesen commented Jul 11, 2024 •

edited

Loading

ewollesen commented Oct 7, 2024 •

edited

Loading

ewollesen Feb 18, 2025 •

edited

Loading

ewollesen Feb 14, 2025 •

edited

Loading

toddkazakov Feb 18, 2025 •

edited

Loading

ewollesen Feb 19, 2025 •

edited

Loading

ewollesen Feb 19, 2025 •

edited

Loading

toddkazakov Feb 21, 2025 •

edited

Loading

ewollesen Feb 21, 2025 •

edited

Loading

ewollesen Feb 28, 2025 •

edited

Loading