Skip to content
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

Deprecate Stream Enrich and enrich-rabbitmq #787

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Conversation

spenes
Copy link
Contributor

@spenes spenes commented May 2, 2023

No description provided.

@spenes spenes force-pushed the deprecate-stream-enrich branch 2 times, most recently from 9738559 to 84868e0 Compare May 2, 2023 09:55
@spenes spenes changed the title Deprecate stream-enrich Deprecate Stream Enrich May 2, 2023
Copy link
Contributor

@benjben benjben left a comment

Choose a reason for hiding this comment

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

Great work @spenes !

I wonder if we should merge common/ and common-fs2. On one hand it's probably good to keep common/ as is with the just the core logic of the adapters/enrichments. But on the other hand it sounds weird to have two modules with some common logic. Given that Scala Common Enrich is published as a library to Maven in case someone wants to use it in a custom app, I think that we should leave it as is.

@benjben benjben force-pushed the deprecate-stream-enrich branch 2 times, most recently from ac8088a to 780d4c8 Compare May 23, 2023 10:01
@benjben benjben force-pushed the deprecate-stream-enrich branch 2 times, most recently from 18e570d to e3c2bdc Compare September 4, 2023 15:01
@benjben benjben force-pushed the deprecate-stream-enrich branch 5 times, most recently from 50fea9f to 09b7994 Compare October 18, 2023 09:08
@benjben benjben changed the title Deprecate Stream Enrich Deprecate Stream Enrich and enrich-rabbitmq Oct 18, 2023
@benjben benjben force-pushed the deprecate-stream-enrich branch 3 times, most recently from 503316e to fba35f8 Compare October 20, 2023 13:58
@benjben benjben force-pushed the deprecate-stream-enrich branch from fba35f8 to 4a9f950 Compare October 23, 2023 07:36
.value
.unsafeRunSync()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that but changing this would require a lot of refactoring in the tests. Instead I would prefer us to make it possible to create an IgluCirceClient manually without having to parse a config file, so that we don't get a IO[ ].

@benjben
Copy link
Contributor

benjben commented Oct 23, 2023

Apart from removing Stream Enrich and enrich-rabbitmq, I took the opportunity to do some cleaning in order to make the code easier to understand.

  • HttpClient is now passed explicity as a parameter to the places that need it, rather than being passed implicitly everywhere. This makes it easier to follow where it is initialized, and it is much more clear what uses the HTTP client. For instance a RemoteAdapter now has its own HTTP client defined in the class, rather than passing it implicity when we convert to a RawEvent.

  • Now the only implicit s are the ones from Cats Effect and from outside Enrich codebase (e.g. RegistryLookup). For instance

def build[F[_]: Monad: CreateForex: CreateIabClient: CreateIpLookups: CreateOWM: CreateParser: CreateUaParserEnrichment: sqlquery.CreateSqlQueryEnrichment: apirequest.CreateApiRequestEnrichment]

became

def build[F[_]: Async: Clock: ContextShift]
  • I did some cleaning in the package names, so that it's more clear where imports come from. For instance
package com.snowplowanalytics.snowplow.enrich.common
package adapters
package registry

became

package com.snowplowanalytics.snowplow.enrich.common.adapters.registry
  • A cached thread pool is now used for the blocking calls of the unit tests

  • I removed cats.Id entirely from the codebase. This led to many changes in the tests because now all the results are wrapped in the effect.

Copy link
Contributor Author

@spenes spenes left a comment

Choose a reason for hiding this comment

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

Looks good to me! However, Github doesn't allow me to approve the PR since I am the one opened the PR. But, from my point of view, it is good to go.

@benjben benjben merged commit 29f2672 into develop Oct 24, 2023
1 check passed
@benjben benjben deleted the deprecate-stream-enrich branch October 24, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants