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

Log elasticsearch issues in any case #2035

Closed
wants to merge 1 commit into from

Conversation

JoshuaBehrens
Copy link
Contributor

@JoshuaBehrens JoshuaBehrens commented Aug 24, 2021

1. Why is this change necessary?

When you run in prod mode and have an elastichsearch falling back to database search you don't know why because neither a log message is logged or an exception is thrown.

2. What does this change do, exactly?

Log in any case of an exception before environment evaluation is done.

3. Describe each step to reproduce the issue or behaviour.

  1. Use plugins that extend elasticsearch queries
  2. Have a slow page or weird behaviour when enabling elastic based search
  3. No hints in the log files

4. Checklist

Is there a legit way how to test it when the first check literally is: am I in a test?

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

@JoshuaBehrens
Copy link
Contributor Author

I'll add a changelog file later

@JoshuaBehrens
Copy link
Contributor Author

Is there a legit way how to test it when the first check literally is: am I in a test?

For real? Shouldn't the first be a mock or decorator doing exactly this?

@shyim
Copy link
Member

shyim commented Aug 26, 2021

@JoshuaBehrens You mean with testing? You can construct the class by own and pass a mock to test it

@JoshuaBehrens
Copy link
Contributor Author

@shyim I mean why is there an env check on 'test'? This should not be code within this namespace, right?

@shopwareBot
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-16905

Please use this issue to track the state of your pull request.

@NiklasLimberg
Copy link
Contributor

I'll mark this as incomplete as long as there isn't a changelog file

@NiklasLimberg
Copy link
Contributor

Hi @JoshuaBehrens,
please write a quick changelog for this and make sure to include the ticket number

@JoshuaBehrens
Copy link
Contributor Author

I added a changelog. Reading that changelog makes me think: do we want to rename logOrThrowException to tryToThrow? I mean logging is not an optional case anymore

@shopwareBot
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-17334

Please use this issue to track the state of your pull request.

@NiklasLimberg
Copy link
Contributor

NiklasLimberg commented Sep 17, 2021

well this was already imported to jira hmmmm

@JoshuaBehrens
Copy link
Contributor Author

@NiklasLimberg which one shall I reference in the changelog?

@NiklasLimberg
Copy link
Contributor

@JoshuaBehrens Ideally NEXT-16905, i've closed the otherone as a duplicate already

@JoshuaBehrens
Copy link
Contributor Author

I rebased, refactored the helper into an abstract one, aliased the service id, changed changelog. Can you tell me that it was ok to not pull setEnabled into the contract? This isn't needed anyway when #1821 is resolved, right?

@mitelg did I do that correctly?

@shopwareBot
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-17664

Please use this issue to track the state of your pull request.

@NiklasLimberg
Copy link
Contributor

Just disregard the new issue, I forgot again that it already has an issue

@shyim
Copy link
Member

shyim commented Oct 20, 2021

As we discussed on Slack, remove the abstract class 👍

@JoshuaBehrens
Copy link
Contributor Author

I rebased and amend by consensus

@NiklasLimberg
Copy link
Contributor

Hi @JoshuaBehrens,
i've imported this aswell again @shyim will review it once he is back from vacation next week

@shyim
Copy link
Member

shyim commented Nov 25, 2021

Thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants