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

(executor): Add logging to HttpElasticExecutor #16

Merged
merged 7 commits into from
Dec 14, 2022
Merged

Conversation

dbulaja98
Copy link
Collaborator

No description provided.

Copy link
Member

@drmarjanovic drmarjanovic left a comment

Choose a reason for hiding this comment

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

  1. Does it make more sense to use DEBUG level instead?
  2. What about using logDebug(...) *> request.get(...)?
  3. Should we provide more arguments? Executing create or update document... is not useful too much... Maybe we should provide index, routing, documentId, body, etc.

@arnoldlacko

@arnoldlacko
Copy link
Collaborator

  1. Does it make more sense to use DEBUG level instead?
  2. What about using logDebug(...) *> request.get(...)?
  3. Should we provide more arguments? Executing create or update document... is not useful too much... Maybe we should provide index, routing, documentId, body, etc.

@arnoldlacko

  1. I think that makes sense to use debug
  2. I think both *> and _ <- are fine and we should choose based on what looks better in this case. Given that sending the request is a multi-liner, I think the for-comprehension might be slightly nicer here, unless we extract that part.
  3. I'm thinking that library-level logs could be useful only if we provide information that is not visible through the API. What comes to mind is the raw request we send to ES and the raw response that we receive (with the additional meta-data that we don't expose in the response.

@dbulaja98 dbulaja98 force-pushed the setup-logging branch 2 times, most recently from 4dae96c to 27f2320 Compare December 7, 2022 13:11
arnoldlacko
arnoldlacko previously approved these changes Dec 12, 2022
drmarjanovic
drmarjanovic previously approved these changes Dec 14, 2022
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.

3 participants