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

Application usage statistics #2387

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Application usage statistics #2387

wants to merge 11 commits into from

Conversation

alex-odysseus
Copy link
Contributor

@alex-odysseus alex-odysseus commented Aug 27, 2024 •
edited
Loading

Addressing #2329

Business logic is based on parsing Audit Trail log entries. As some of the Analysis Execution entries were identified as duplicates a filter has been introduced to count each Analysis Execution precisely once

Usage examples (for the second one {} is used in the 'urlPattern' parameter to indicated a dynamic part of the URL):

curl --location --request POST 'localhost:8080/WebAPI/statistic/executions' \
--header 'Content-Type: application/json' \
--header 'Cookie: JSESSIONID=398******************EB0' \
--data-raw '{
"startDate":"2023-11-01",
"endDate":"2023-11-10",
"sourceKey":"SYNPUF_110k"
}'


curl --location --request POST 'localhost:8080/WebAPI/statistic/accesstrends' \
--header 'Content-Type: application/json' \
--header 'Cookie: JSESSIONID=398************************EB0' \
--data-raw '{
"startDate":"2024-03-12",
"endDate":"2024-03-12",
"endpoints":[{"method":"POST","urlPattern":"/WebAPI/conceptset/check"},{"method":"GET","urlPattern":"/WebAPI/i18n/locales"},{"method":"GET","urlPattern":"/WebAPI/cdmresults/{}/dashboard"}],
"responseFormat":"CSV",
"showUserInformation":true
}'

alex-odysseus
alex-odysseus commented Aug 27, 2024
View reviewed changes
src/main/java/org/ohdsi/webapi/statistic/controller/StatisticController.java

@Controller
@Path("/statistic/")
@ConditionalOnProperty(value = "audit.trail.enabled", havingValue = "true")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Audit Trail feature is not enabled the Controller is not efficient

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the intent here to make the /statistic/ end-point available based on the audit.trail.enabled property? Seems a little confusing to make an endpoint conditional vs. having it always available and to have it return based on the status of the configuration of the application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally see no problem that some of the endpoints are not available if an appropriate configuration is not at place

alex-odysseus
alex-odysseus commented Aug 27, 2024
View reviewed changes
src/main/java/org/ohdsi/webapi/statistic/controller/StatisticController.java
private StatisticService service;

public enum ResponseFormat {
CSV, JSON
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both CSV and JSON output formats should be supported

alex-odysseus
alex-odysseus commented Aug 27, 2024
View reviewed changes
src/main/java/org/ohdsi/webapi/statistic/controller/StatisticController.java Outdated
}
}

private void updateExecutionStatisticsHeader(boolean showUserInformation) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both methods to be replaced by extra two static constants!

Copy link
Collaborator

Choose a reason for hiding this comment

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

When will this be replaced?

alex-odysseus
alex-odysseus commented Aug 27, 2024
View reviewed changes
src/main/java/org/ohdsi/webapi/statistic/dto/SourceExecutionDto.java Outdated
private String sourceName;
private String executionName;
private String executionDate;
private String userID;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

userId

Copy link
Collaborator

Choose a reason for hiding this comment

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

When do you plan to change this?

alex-odysseus
alex-odysseus commented Aug 27, 2024
View reviewed changes
src/main/java/org/ohdsi/webapi/statistic/service/StatisticService.java Outdated
protected final Logger LOG = LoggerFactory.getLogger(getClass());

@Value("${audit.trail.log.file}")
// TODO remove value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be configured via properties

Copy link
Collaborator

Choose a reason for hiding this comment

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

So will you remove this then?

alex-odysseus
alex-odysseus commented Aug 27, 2024
View reviewed changes
...migration/postgresql/V2.14.0.20231206190600__fixing_statistics_enpoint_permission_method.sql Outdated
@@ -0,0 +1,2 @@
UPDATE ${ohdsiSchema}.sec_permission SET value='statistic:executions:post'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may combine both scripts into one

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please do and make sure the prefix of the file is v2.15...

@anthonysena anthonysena linked an issue Sep 3, 2024 that may be closed by this pull request
Application usage statistics #2329
Open
anthonysena
anthonysena requested changes Dec 27, 2024
View reviewed changes
src/main/java/org/ohdsi/webapi/statistic/controller/StatisticController.java

@Controller
@Path("/statistic/")
@ConditionalOnProperty(value = "audit.trail.enabled", havingValue = "true")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the intent here to make the /statistic/ end-point available based on the audit.trail.enabled property? Seems a little confusing to make an endpoint conditional vs. having it always available and to have it return based on the status of the configuration of the application.

src/main/java/org/ohdsi/webapi/statistic/controller/StatisticController.java Show resolved Hide resolved
src/main/java/org/ohdsi/webapi/statistic/controller/StatisticController.java Outdated
}
}

private void updateExecutionStatisticsHeader(boolean showUserInformation) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When will this be replaced?

src/main/java/org/ohdsi/webapi/statistic/dto/SourceExecutionDto.java Outdated
private String sourceName;
private String executionName;
private String executionDate;
private String userID;
Copy link
Collaborator

Choose a reason for hiding this comment

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

When do you plan to change this?

src/main/java/org/ohdsi/webapi/statistic/service/StatisticService.java Outdated
protected final Logger LOG = LoggerFactory.getLogger(getClass());

@Value("${audit.trail.log.file}")
// TODO remove value
Copy link
Collaborator

Choose a reason for hiding this comment

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

So will you remove this then?

...migration/postgresql/V2.14.0.20231206190600__fixing_statistics_enpoint_permission_method.sql Outdated
@@ -0,0 +1,2 @@
UPDATE ${ohdsiSchema}.sec_permission SET value='statistic:executions:post'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please do and make sure the prefix of the file is v2.15...

@alex-odysseus
Log entry added, non-closed resources are correctly handled, file hea…
831e3e7 
…der collections are stateless, hard-coded constants deleted relying on the application configuration file

Migration scripts combined
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthonysena anthonysena anthonysena requested changes

@chrisknoll chrisknoll Awaiting requested review from chrisknoll

Requested changes must be addressed to merge this pull request.

Assignees

@anthonysena anthonysena

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Application usage statistics
4 participants
@alex-odysseus @anthonysena @ssuvorov-fls @hernaldourbina