-
Notifications
You must be signed in to change notification settings - Fork 286
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
fix(cmd-api-server): stop changing LoggerProvider log level #3362
Merged
petermetz
merged 1 commit into
hyperledger-cacti:main
from
petermetz:fix-cmd-api-server-global-log-level-override
Jul 1, 2024
Merged
fix(cmd-api-server): stop changing LoggerProvider log level #3362
petermetz
merged 1 commit into
hyperledger-cacti:main
from
petermetz:fix-cmd-api-server-global-log-level-override
Jul 1, 2024
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
petermetz
requested review from
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna,
sandeepnRES and
outSH
as code owners
June 26, 2024 20:09
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Jun 27, 2024
1. The logs were showing the bound host which in our case was 0.0.0.0 but that is not accepted by web browsers when you are executing HTTP XHR requests from Javascript. 2. This change makes it so that even though we bind to 0.0.0.0 the logs to the person running the example application will show 127.0.0.1. This might potentially cause further confusion in some people who'd think that we are binding to 127.0.0.1 based on the logs, but this seems like an acceptable trade-off for an example which has the number one priority of being easily digestable. Fixes hyperledger-cacti#2390 Depends on hyperledger-cacti#3362 Signed-off-by: Peter Somogyvari <[email protected]>
5 tasks
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Jun 27, 2024
1. The logs were showing the bound host which in our case was 0.0.0.0 but that is not accepted by web browsers when you are executing HTTP XHR requests from Javascript. 2. This change makes it so that even though we bind to 0.0.0.0 the logs to the person running the example application will show 127.0.0.1. This might potentially cause further confusion in some people who'd think that we are binding to 127.0.0.1 based on the logs, but this seems like an acceptable trade-off for an example which has the number one priority of being easily digestable. Fixes hyperledger-cacti#2390 Depends on hyperledger-cacti#3362 Signed-off-by: Peter Somogyvari <[email protected]>
jagpreetsinghsasan
approved these changes
Jun 27, 2024
outSH
approved these changes
Jul 1, 2024
1. The API server was mutating global shared state in it's own constructor which was causing problems with other components (pretty much all of them) 2. I deleted the line that copies the API server's own log level to the LoggerProvider so that the API server's log level can be it's own. 3. The way this bug came about is that in the supply chain app example in the back-end the API server had to be muted (log level WARN) in order for it to stop printing misleading logs due to us binding to the wildcard host it would claim in its own logs that the WWW GUI is accessible on the wildcard host in a web browser, but this was wrong and caused many people a lot of confusion unfortunately. 4. The fix for the supply chain app is to set the API server's log level to WARN and have the supply chain app itself log the correct URLs to the console. 5. The issue I ran into with that fix is that as soon as I set the log level of the API server to WARN, everything else also stopped logging which resulted in my fix making everything worse since now the users had absolutely no idea what was happening or if the example application had even finished booting up or not. 6. Upon further debugging I discovered that the API server was forcing its own log level onto everybody else as the root cause. 7. A follow-up PR is about to drop with the supply chain app fixes which are dependent on this one making it in first. Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
force-pushed
the
fix-cmd-api-server-global-log-level-override
branch
from
July 1, 2024 16:34
e267907
to
bb7b7a9
Compare
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Jul 1, 2024
1. The logs were showing the bound host which in our case was 0.0.0.0 but that is not accepted by web browsers when you are executing HTTP XHR requests from Javascript. 2. This change makes it so that even though we bind to 0.0.0.0 the logs to the person running the example application will show 127.0.0.1. This might potentially cause further confusion in some people who'd think that we are binding to 127.0.0.1 based on the logs, but this seems like an acceptable trade-off for an example which has the number one priority of being easily digestable. Fixes hyperledger-cacti#2390 Depends on hyperledger-cacti#3362 Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
added a commit
that referenced
this pull request
Jul 1, 2024
1. The logs were showing the bound host which in our case was 0.0.0.0 but that is not accepted by web browsers when you are executing HTTP XHR requests from Javascript. 2. This change makes it so that even though we bind to 0.0.0.0 the logs to the person running the example application will show 127.0.0.1. This might potentially cause further confusion in some people who'd think that we are binding to 127.0.0.1 based on the logs, but this seems like an acceptable trade-off for an example which has the number one priority of being easily digestable. Fixes #2390 Depends on #3362 Signed-off-by: Peter Somogyvari <[email protected]>
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.
which was causing problems with other components (pretty much all of them)
LoggerProvider so that the API server's log level can be it's own.
in the back-end the API server had to be muted (log level WARN) in order
for it to stop printing misleading logs due to us binding to the wildcard
host it would claim in its own logs that the WWW GUI is accessible on the
wildcard host in a web browser, but this was wrong and caused many people
a lot of confusion unfortunately.
WARN and have the supply chain app itself log the correct URLs to the console.
of the API server to WARN, everything else also stopped logging which resulted
in my fix making everything worse since now the users had absolutely no
idea what was happening or if the example application had even finished booting
up or not.
own log level onto everybody else as the root cause.
dependent on this one making it in first.
Signed-off-by: Peter Somogyvari [email protected]
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.