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.

Sign up for GitHub

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

Jump to bottom

ISPN-14766 Integrate with Insights #10929

Merged
merged 1 commit into from Sep 21, 2023
Merged

ISPN-14766 Integrate with Insights #10929

merged 1 commit into from Sep 21, 2023

Conversation

fax4ever
Copy link
Contributor

@fax4ever fax4ever commented May 10, 2023 •
edited

wburns
wburns reviewed May 22, 2023
View reviewed changes
core/src/main/java/org/infinispan/util/concurrent/BlockingManagerImpl.java Show resolved Hide resolved
core/src/main/java/org/infinispan/util/concurrent/BlockingManagerImpl.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/InsightsModule.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/report/InfinispanSubreport.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/report/InfinispanTopReport.java Outdated Show resolved Hide resolved
...ghts/src/main/java/org/infinispan/server/insights/scheduler/InfinispanInsightsScheduler.java Outdated Show resolved Hide resolved
...ghts/src/main/java/org/infinispan/server/insights/scheduler/InfinispanInsightsScheduler.java Show resolved Hide resolved
@fax4ever
Copy link
Contributor Author

Thank you @wburns for the review.
I should have addressed all the points here.
I also added a commit to disable the integration from the module by default, the integration will be active only if the system property infinispan.insights.enabled is set to true.

@fax4ever
Copy link
Contributor Author

@tristantarrant I updated the dependency of Java Insights client to 1.0.9.
Always building it locally from the 1.0.9 tag.

tristantarrant
tristantarrant requested changes Jul 17, 2023
View reviewed changes
Copy link
Member

@tristantarrant tristantarrant left a comment

Choose a reason for hiding this comment

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

The Infinispan report needs to be changed to include higher-level info, such as cache features in uses, realm types, whether TLS is used at the endpoint and transport level, which protocols are being used, etc

...r/insights/src/main/java/org/infinispan/server/insights/InfinispanInsightsConfiguration.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/report/InfinispanTopReport.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/report/InfinispanSubreport.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/report/InfinispanSubreport.java Outdated Show resolved Hide resolved
@fax4ever fax4ever mentioned this pull request Jul 19, 2023
Closed
@fax4ever
Copy link
Contributor Author

fax4ever commented Jul 19, 2023 •
edited

Thanks @tristantarrant I applied the changes and opened this other pull request to generate and publish the new report: #11137

@fax4ever fax4ever force-pushed the ISPN-14766 branch 11 times, most recently from 2b5a18e to cf2e0b7 Compare July 26, 2023 15:40
@fax4ever fax4ever force-pushed the ISPN-14766 branch 7 times, most recently from b05ea25 to 97c3631 Compare August 1, 2023 14:17
@fax4ever fax4ever marked this pull request as ready for review August 2, 2023 08:29
@fax4ever
Copy link
Contributor Author

fax4ever commented Aug 2, 2023 •
edited

@tristantarrant I think I've made the changes requested

tristantarrant
tristantarrant requested changes Sep 11, 2023
View reviewed changes
server/insights/src/main/java/org/infinispan/server/insights/logging/Log.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/logging/Log.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/logging/Log.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/report/InfinispanSubreport.java Outdated Show resolved Hide resolved
server/insights/src/main/java/org/infinispan/server/insights/InsightsModule.java Outdated Show resolved Hide resolved
server/core/src/main/java/org/infinispan/server/core/BaseServerManagement.java Outdated Show resolved Hide resolved
server/core/src/main/java/org/infinispan/server/core/BaseServerManagement.java Outdated Show resolved Hide resolved
server/core/src/main/java/org/infinispan/server/core/BaseServerManagement.java Outdated Show resolved Hide resolved
server/core/src/main/java/org/infinispan/server/core/BaseServerManagement.java Show resolved Hide resolved
server/core/src/main/java/org/infinispan/server/core/BaseServerManagement.java Outdated Show resolved Hide resolved
@fax4ever
Copy link
Contributor Author

@tristantarrant thanks for the review.
I think I applied all the suggestions here.

@fax4ever
Copy link
Contributor Author

Made the last changes and rebased.

@fax4ever fax4ever force-pushed the ISPN-14766 branch 3 times, most recently from d9b2a77 to e2a8ba3 Compare September 13, 2023 04:40
@ryanemerson ryanemerson changed the title JDG-5898 Integrate with Insights ISPN-14766 Integrate with Insights Sep 20, 2023
@ryanemerson
Copy link
Contributor

@fax4ever Can you remove the commit adding -Pinsights and squash the commits, then I think we're good to go.

@fax4ever
Copy link
Contributor Author

@fax4ever Can you remove the commit adding -Pinsights and squash the commits, then I think we're good to go.

Sure @ryanemerson. Thanks for review it!
I'm rebasing now. After that I'll push the changes.
If after that CI works, I'll remove the commit adding -Pinsights and squash the commits.

@fax4ever
Copy link
Contributor Author

fax4ever commented Sep 21, 2023 •
edited

@ryanemerson I also added ISPN-14766 Apply ISPN-15143 to insights module to make the Insight module consistent with the change of ISPN-15143.

@fax4ever
Copy link
Contributor Author

@ryanemerson it seems that all still work.
See https://ci.infinispan.org/blue/organizations/jenkins/Infinispan/detail/PR-10929/34/pipeline.
Going to squash the commits the removing the temp one.
Could you merge it after that?

@fax4ever
Copy link
Contributor Author

@fax4ever Can you remove the commit adding -Pinsights and squash the commits, then I think we're good to go.

done!

@tristantarrant tristantarrant merged commit eb7e866 into infinispan:main Sep 21, 2023
3 of 4 checks passed
@tristantarrant
Copy link
Member

Merged, thanks

@fax4ever
Copy link
Contributor Author

Thank you @tristantarrant !!!

@fax4ever fax4ever deleted the ISPN-14766 branch September 22, 2023 08:16
@fax4ever fax4ever mentioned this pull request Oct 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tristantarrant tristantarrant tristantarrant requested changes

@wburns wburns wburns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@fax4ever @ryanemerson @tristantarrant @wburns