Choreo analytics for Micro Integrator. #2927
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ted issues Related to wso2/api-manager#1992
Related to wso2/api-manager#1992
Related to wso2/api-manager#1992
Related to wso2/api-manager#1992
Related to wso2/api-manager#1992
senthuran16
requested changes
Jul 19, 2023
Comment on lines
40
to
53
| if (publisherTypes != null && !publisherTypes.isEmpty() && | ||
| (publisherTypes.contains(MediationDataPublisherConstants.DATABRIDGE_PUBLISHER_TYPE ) | ||
| || publisherTypes.contains(MediationDataPublisherConstants.LOG_PUBLISHER_TYPE))) { | ||
| || publisherTypes.contains(MediationDataPublisherConstants.LOG_PUBLISHER_TYPE) | ||
| || publisherTypes.contains(MediationDataPublisherConstants.CHOREO_PUBLISHER_TYPE))) { | ||
| if (publisherTypes.contains(MediationDataPublisherConstants.DATABRIDGE_PUBLISHER_TYPE)) { | ||
| statPublishers.add(EIStatisticsPublisher.GetInstance()); | ||
| } | ||
| if (publisherTypes.contains(MediationDataPublisherConstants.LOG_PUBLISHER_TYPE)) { | ||
| statPublishers.add(ElasticStatisticsPublisher.GetInstance()); | ||
| } | ||
| if (publisherTypes.contains(MediationDataPublisherConstants.CHOREO_PUBLISHER_TYPE)) { | ||
| statPublishers.add(ChoreoStatisticsPublisher.GetInstance()); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Shall we optimize this if condition, something like:
boolean useDefaultPublisher = true;
if (publisherTypes != null && !publisherTypes.isEmpty()) {
if (publisherTypes.contains(MediationDataPublisherConstants.DATABRIDGE_PUBLISHER_TYPE)) {
statPublishers.add(EIStatisticsPublisher.GetInstance());
useDefaultPublisher = false;
}
// Other publisherTypes go here
}
if (useDefaultPublisher) {
statPublishers.add(ElasticStatisticsPublisher.GetInstance());
}There was a problem hiding this comment.
Optimised as requested by 7ce08a2
Comment on lines
20
to
55
| /* | ||
| * Documentation | ||
| * | ||
| * Enable Choreo Analytics | ||
| * | ||
| * 1. Add the following to the deployment.toml file. | ||
| * | ||
| * i. Enabling statistics for artifacts | ||
| * [mediation] | ||
| * flow.statistics.enable=true | ||
| * flow.statistics.capture_all=true | ||
| * | ||
| * ii. Enabling Choreo Analytics | ||
| * [analytics] | ||
| * enabled=true | ||
| * publisher="choreo" | ||
| * | ||
| * **Adding publisher is required. If not added, the default publisher will be used which is ELK flow** | ||
| * | ||
| * 2. Optionally, you can add the following to the deployment.toml file. | ||
| * | ||
| * [analytics] | ||
| * api_analytics.enabled = true | ||
| * proxy_service_analytics.enabled = true | ||
| * sequence_analytics.enabled = true | ||
| * endpoint_analytics.enabled = true | ||
| * inbound_endpoint_analytics.enabled = true | ||
| * | ||
| * Note that default value for above 5 will be true (If not added). If you want to disable analytics for any of the | ||
| * above, change the value to false. | ||
| * | ||
| * 3. Restart the server. | ||
| * | ||
| * Special Note - This is a WIP code so publishing to Choreo is not fully implemented. | ||
| * | ||
| */ |
There was a problem hiding this comment.
Let's move this to a class comment
There was a problem hiding this comment.
Moved to a class comment by 7ce08a2
Comment on lines
84
to
88
| private boolean analyticsEnabledForAPI; | ||
| private boolean analyticsEnabledForSequences; | ||
| private boolean analyticsEnabledForProxyServices; | ||
| private boolean analyticsEnabledForEndpoints; | ||
| private boolean analyticsEnabledForInboundEndpoints; |
There was a problem hiding this comment.
Shall we rename these as isApiAnalyticsEnabled, isSequenceAnalyticsEnabled and likewise?
There was a problem hiding this comment.
Renamed by 7ce08a2
| */ | ||
| @Override | ||
| public void process(PublishingFlow publishingFlow, int tenantId) { | ||
| if (!analyticsEnabled) { |
There was a problem hiding this comment.
It seems we aren't setting the value of analyticsEnabled anywhere. Am I missing something here?
There was a problem hiding this comment.
In line 115 inside loadConfigurations(), we read the configurations. This will be the value of
[analytics]
enabled=true
of deployment.toml
| if (!analyticsEnabled) { | ||
| return; | ||
| } | ||
| if (publishingFlow.getEvents().toArray().length > 0) { |
There was a problem hiding this comment.
Will publishingFlow.getEvents() return null by any chance? If so, let's do a null check
There was a problem hiding this comment.
Added a null check by 7ce08a2
| private JsonObject choreoPayload (PublishingEvent event) { | ||
| try { | ||
| ElasticMetadata metadata = event.getElasticMetadata(); | ||
| JsonObject pL = new JsonObject(); |
There was a problem hiding this comment.
Renamed by 7ce08a2
Comment on lines
144
to
162
| if ((StatisticsConstants.FLOW_STATISTICS_API.equals(event.getComponentType()) && | ||
| analyticsEnabledForAPI) || | ||
| (StatisticsConstants.FLOW_STATISTICS_SEQUENCE.equals(event.getComponentType()) && | ||
| analyticsEnabledForSequences) || | ||
| (StatisticsConstants.FLOW_STATISTICS_ENDPOINT.equals(event.getComponentType()) && | ||
| analyticsEnabledForEndpoints) || | ||
| (StatisticsConstants.FLOW_STATISTICS_INBOUNDENDPOINT.equals(event.getComponentType()) && | ||
| analyticsEnabledForInboundEndpoints) || | ||
| (StatisticsConstants.FLOW_STATISTICS_PROXYSERVICE.equals(event.getComponentType()) && | ||
| analyticsEnabledForProxyServices)) { | ||
| if ((StatisticsConstants.FLOW_STATISTICS_SEQUENCE.equals(event.getComponentType()) && | ||
| event.getElasticMetadata().getSequence(event.getComponentName()) == null) || | ||
| (StatisticsConstants.FLOW_STATISTICS_ENDPOINT.equals(event.getComponentType()) && | ||
| event.getElasticMetadata().getEndpoint(event.getComponentName()) == null)) { | ||
| // If Endpoint or Sequence is null | ||
| return; | ||
| } | ||
| publishAnalytics(event); | ||
| } |
There was a problem hiding this comment.
Shall we do early exits?
Something like:
for eg:
if ((StatisticsConstants.FLOW_STATISTICS_API.equals(event.getComponentType()) &&
!analyticsEnabledForAPI) {
return;
}
// Similarly for PROXY and INBOUND_ENDPOINT
if ((StatisticsConstants. FLOW_STATISTICS_SEQUENCE.equals(event.getComponentType()) {
if (!analyticsEnabledForAPI ||
event.getElasticMetadata().getSequence(event.getComponentName()) == null)) {
return;
}
}
// Similarly for ENDPOINT
publishAnalytics(event);
There was a problem hiding this comment.
Changed by 7ce08a2.
Since we have to publishing Analytics for only 5 artefacts, publishAnalytics(event) added inside each if cases.
| * @param event This is the event published by synapse. | ||
| * @return This returns a JsonObject which contains the required flat Json by Choreo. | ||
| */ | ||
| private JsonObject choreoPayload (PublishingEvent event) { |
There was a problem hiding this comment.
Let's rename this to generateChoreoPayload
There was a problem hiding this comment.
Renamed by publishAnalytics(event)
2 tasks
senthuran16
approved these changes
Jul 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Fix wso2/api-manager#1992
Steps to enable Choreo Analytics
i. Enabling statistics for artifacts
ii. Enabling Choreo Analytics
Adding publisher is required. If not added, the default publisher will be used which is ELK flow
Note that default value for above 5 will be true (If not added). If you want to disable analytics for any of the above, change the value to false.
Special Note - This is a WIP code so publishing to Choreo is not fully implemented.