Skip to content

Add customizer interface for spring boot integration #434

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

Closed
wants to merge 3 commits into from

Conversation

koenpunt
Copy link

This to allow customization of the client, without having to manually create the client.

This also includes the changes from #433.

Comment on lines +25 to +27
Analytics.Builder builder = Analytics.builder(properties.getWriteKey());
customizerProvider.orderedStream().forEach((customizer) -> customizer.customize(builder));
return builder.build();
Copy link
Author

Choose a reason for hiding this comment

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

@edsonjab
Copy link
Contributor

Hi @koenpunt all your changes looks pretty well, could you help us to run mvn spotless:apply to fix the tests, please?

@koenpunt koenpunt force-pushed the spring-boot-customizer branch from 671aec9 to d713c82 Compare April 25, 2023 18:57
@koenpunt
Copy link
Author

@edsonjab done!

Copy link
Contributor

@edsonjab edsonjab left a comment

Choose a reason for hiding this comment

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

All works.

@koenpunt koenpunt force-pushed the spring-boot-customizer branch from d713c82 to a39b5a2 Compare May 16, 2023 11:10
@MichaelGHSeg
Copy link
Contributor

Our development and maintenance efforts have shifted to the new Analytics-Kotlin library. We suggest migrating when you can and we are more likely to accept interface suggestions over there. You're also welcome to continue using your fork of this repo of course :)

@koenpunt
Copy link
Author

We've used the analytics kotlin library before, but it's missing features and we actually switched to the Java library. I think as long as the kotlin library is mobile device oriented and keeps state during the lifetime of an instance the java project shouldn't be abandoned.

@koenpunt
Copy link
Author

koenpunt commented Jun 2, 2023

Also, Java-only projects are still a thing, and I can't imagine them adding kotlin just for the Segment integration. @MichaelGHSeg it would be greatly appreciated if this could be reconsidered.

@didiergarcia
Copy link

We've used the analytics kotlin library before, but it's missing features and we actually switched to the Java library. I think as long as the kotlin library is mobile device oriented and keeps state during the lifetime of an instance the java project shouldn't be abandoned.

Hi @koenpunt, please let us know what features are missing in the analytics-kotlin version. We're super interested in making switching easier.

As far as adding analytics-kotlin I think it's only kotlin-stdlib, kotlin-coroutines, and analytics-kotlin that would be needed.

We recently added a analytics.shutdown() for clean termination in containerized environments and will be working on an enhancement to allow the library to work in a stateless manner later this year.

@koenpunt
Copy link
Author

koenpunt commented Jun 6, 2023

I think there is too much missing for the Java library to be put in the background. We switched to the Java library, so can't really give any feedback on the kotlin library.
But the main problem with the kotlin library is that it maintains state, which in my opinion only makes sense in an (mobile) app environment, and not a server environment.
And in regards to the kotlin library for Java only projects; it doesn't make sense to require people to add kotlin libraries just for segment, especially when there's fully functional (if not better) Java implementation.

I do think both should be able to exists, and one shouldn't be replacing the other.

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.

4 participants