Skip to content

feat(echo): set log level according to env #225

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
Merged
merged 2 commits into from
Apr 16, 2024
Merged

feat(echo): set log level according to env #225

merged 2 commits into from
Apr 16, 2024

Conversation

subpop
Copy link
Collaborator

@subpop subpop commented Apr 3, 2024

Set the log level according to the value of YGG_LOG_LEVEL.
Print the worker's environment when run at level Debug or higher.

@subpop subpop requested a review from jirihnidek April 3, 2024 13:57
@jirihnidek
Copy link
Contributor

I had some issues with testing echo worker for any branch forked from yggdrasil-0.2, because instructions in HACKING.md are not correct.

  1. Command for getting current consumer UUID is not correct:
export CONSUMER_ID=$(openssl x509 -in cert.pem -subject -nocert | cut -f3 -d" ")

because it the path to cert.pem is relative path. It should be absolute path (/etc/pki/consumer/cert.pem). If this is fixed, then environment variable contains , at the end. The command has to be run using sudo.

When fixed, then it should be something like this:

export CONSUMER_ID=$(sudo openssl x509 -in /etc/pki/consumer/cert.pem -subject -nocert | cut -f3 -d" " | cut -c-36)
  1. Command for sending message is wrong. It should be probably something like this:
mosquitto_pub --host 127.0.0.1 --port 1883 --topic yggdrasil/${CONSUMER_ID}/data/in --message "{\"type\":\"data\",\"message_id\":\"$(uuidgen | tr -d '\n')\", \"response_to\":\" \",\"version\":1,\"sent\":\"$(date --iso-8601=seconds --utc | tr -d '\n')\",\"directive\":\"echo\",\"content\":\"hello world\"}"
  1. Installing echo worker using sudo install -D -m 755 echo /usr/libexec/yggdrasil/echo-worker is not what you want, because when yggd tries to dispatch message, then it says that there is no echo worker...
[yggd] 2024/04/04 17:24:54 /home/jhnidek/github/redhat-insights/yggdrasil-0.2/cmd/yggd/mqtt.go:18: received a message on topic yggdrasil/ca38df52-c56a-47af-96dc-2416b819fba4/data/in
[yggd] 2024/04/04 17:24:54 /home/jhnidek/github/redhat-insights/yggdrasil-0.2/cmd/yggd/mqtt.go:25: message: 363a07d1-eedc-4f3f-8bf6-94223b61005f
[yggd] 2024/04/04 17:24:54 /home/jhnidek/github/redhat-insights/yggdrasil-0.2/cmd/yggd/grpc.go:125: cannot route message to directive: echo

The echo worker should be installed using:

sudo install -D -m 755 echo /usr/local/libexec/yggdrasil/echo-worker

When echo worker is installed to /usr/local/libexec/yggdrasil/echo-worker, then it seems that it works as expected.

@subpop
Copy link
Collaborator Author

subpop commented Apr 5, 2024

Are these changes introduced by this PR? If not, can you submit a PR to update HACKING?

@jirihnidek
Copy link
Contributor

Are these changes introduced by this PR? If not, can you submit a PR to update HACKING?

Sure I will create PR... I just wanted to add some notes about it somewhere. And the comment was the right place for the case somebody else want to test it.

@jirihnidek
Copy link
Contributor

The PR with updated instructions: #227

jirihnidek
jirihnidek requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

Overall good. I have only two questions/suggestions.

worker/echo/main.go
if logLevel, ok := os.LookupEnv("YGG_LOG_LEVEL"); ok {
level, err := log.ParseLevel(logLevel)
if err != nil {
log.Fatalf("cannot parse log level: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

When log level could not be parsed, is it good idea to set this wrong log level. Shouldn't we skip skip setting log level in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

log.Fatal will print the message string and then exit the program, so here we are not setting an invalid level. The program is exiting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't know that. It is then OK.

subpop added 2 commits April 16, 2024 08:50
@subpop
feat(echo): set log level according to env
861d2ca 
Set the log level according to the value of YGG_LOG_LEVEL.
Print the worker's environment when run at level Debug or higher.

Signed-off-by: Link Dupont <link@sub-pop.net>
@subpop
feat(worker): Add debug logging to startWorker
b7ec70c 
Add more debug logging to the startWorker and loadWorkerConfig
functions.

Signed-off-by: Link Dupont <link@sub-pop.net>
@subpop subpop force-pushed the echo-log-yggdrasil-0.2 branch from 74800f1 to b7ec70c Compare April 16, 2024 12:50
@subpop subpop requested a review from jirihnidek April 16, 2024 12:50
jirihnidek
jirihnidek approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

LGTM

worker/echo/main.go
if logLevel, ok := os.LookupEnv("YGG_LOG_LEVEL"); ok {
level, err := log.ParseLevel(logLevel)
if err != nil {
log.Fatalf("cannot parse log level: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't know that. It is then OK.

@jirihnidek jirihnidek merged commit d8110a5 into RedHatInsights:yggdrasil-0.2 Apr 16, 2024
@subpop subpop deleted the echo-log-yggdrasil-0.2 branch July 11, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jirihnidek jirihnidek jirihnidek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@subpop @jirihnidek