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

[BUG] Subject Area OMAS REST services not using RESTCallLogger #7001

Closed
1 task done
Raunak-S opened this issue Oct 17, 2022 · 6 comments
Closed
1 task done

[BUG] Subject Area OMAS REST services not using RESTCallLogger #7001

Raunak-S opened this issue Oct 17, 2022 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@Raunak-S
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Currently the Subject Area OMAS REST services are using the sl4j Logger instead of the odpi-native RESTCallLogger.

This is a child issue of #6727 to capture specific discussions around the Subject Area OMAS while switching from the default Logger to the RESTCallLogger.

Expected Behavior

The Subject Area OMAS server should use the RESTCallLogger to log REST calls instead of the current Logger.

Steps To Reproduce

No response

Environment

- Egeria:
- OS:
- Java:
- Browser (for UI issues):
- Additional connectors and integration:

Any Further Information?

No response

@Raunak-S Raunak-S added bug Something isn't working triage New bug/issue which needs checking & assigning labels Oct 17, 2022
@Raunak-S
Copy link
Contributor Author

Discussed with @davidradl :

For some of the REST calls, there was an extra guid variable being logged with the usual serverName and userId that isn't supported in the new RESTCallLogger .

Final decision is that this change is fine, since most of the time the guid is the last segment of the URL and will be logged through the new REST logging for those that are familiar with the shape of the APIs

@Raunak-S
Copy link
Contributor Author

Raunak-S commented Oct 21, 2022

I've created a PR to address the main issue, however I've kept it as a draft for now because there were some further changes that could be made that might also fit into this PR. Two possible changes:

  1. The SubjectAreaGlossaryRESTServices service does not have any logging for a few of the methods. If desired, I can add REST call logging to these methods.
  2. The SubjectAreaRelationshipRESTServices service does not have any logging for its methods. If desired, I can add REST call logging to these methods.

Open to any feedback here, thanks! cc: @davidradl

@davidradl
Copy link
Member

@Raunak-S sorry I did not see the pr or the last update. If you can tag me in any updates I hopefully will see them. The change looks good. I can merge as is (once the code has been updated) - it is a good idea to add the calls to the 2 other classes - as you are here and thinking about this. I agree on your conclusion with the guid.

@Raunak-S
Copy link
Contributor Author

Raunak-S commented Nov 4, 2022

@davidradl no problem, I've added the logging for the SubjectAreaGlossaryRESTServices class and switched the PR from draft to ready for review. I've decided to hold off on adding logging to the SubjectAreaRelationshipRESTServices class because it would require a large amount of extra refactoring (there are patterns, i.e. exception handling, that appear in other classes but not this one). I plan to open an enhancement issue for future reference on this class.

@davidradl
Copy link
Member

Thanks @Raunak-S I have merged - are we ok to close this issue?

@Raunak-S
Copy link
Contributor Author

Hi @davidradl yes this issue is all set, I'll close now and update the parent issue, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants