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

GMC: enhance log #371

Merged
merged 12 commits into from
Sep 11, 2024
Merged

GMC: enhance log #371

merged 12 commits into from
Sep 11, 2024

Conversation

KfreeZ
Copy link
Collaborator

@KfreeZ KfreeZ commented Aug 30, 2024

Description

Use standard log to replace the fmt.Printf functions
use curl -X GET "http://localhost:8008/loglevel?level=debug" and curl -X GET "http://localhost:8008/loglevel?level=info" to change the debug level

root@cis-gms-worker-3:~/zkf/repo/kfreez/GenAIInfra/microservices-connector# curl -X PUT "http://localhost:8008/loglevel" -d '{"lloglevel":"debugh"}' -H "Content-Type: application/json"
log_level field is required

root@cis-gms-worker-3:~/zkf/repo/kfreez/GenAIInfra/microservices-connector# curl -X PUT "http://localhost:8008/loglevel" -d '{"loglevel":"debugh"}' -H "Content-Type: application/json"
log_level field is required

root@cis-gms-worker-3:~/zkf/repo/kfreez/GenAIInfra/microservices-connector# curl -X PUT "http://localhost:8008/loglevel" -d '{"log_level":"debugh"}' -H "Content-Type: application/json"
invalid log level: unrecognized level: "debugh"

root@cis-gms-worker-3:~/zkf/repo/kfreez/GenAIInfra/microservices-connector# curl -X PUT "http://localhost:8008/loglevel" -d '{"log_level":"debug"}' -H "Content-Type: application/json"
log level set to debug
root@cis-gms-worker-3:~/zkf/repo/kfreez/GenAIInfra/microservices-connector# curl -X PUT "http://localhost:8008/loglevel" -d '{"log_level":"info"}' -H "Content-Type: application/json"
log level set to info
root@cis-gms-worker-3:~/zkf/repo/kfreez/GenAIInfra/microservices-connector# 

Issues

n/a.

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)

Dependencies

n/a.

Tests

no new feature, CI/CD covers the regression tests

KfreeZ added 2 commits August 30, 2024 12:18
Signed-off-by: KfreeZ <[email protected]>
Signed-off-by: KfreeZ <[email protected]>
@KfreeZ KfreeZ requested review from zhlsunshine and irisdingbj and removed request for zhlsunshine August 30, 2024 05:19
return true
}
if !reflect.DeepEqual(oldObject.DeletionTimestamp, newObject.DeletionTimestamp) {
fmt.Printf("Metadata changes detected, DeletionTimestamp changed from %v to %v\n", oldObject.DeletionTimestamp, newObject.DeletionTimestamp)
_log.Info("Metadata.DeletionTimestamp changes detected", "old", oldObject.DeletionTimestamp, "new", newObject.DeletionTimestamp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall this be on Debug level instead of Info level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from my view, this information is kind of useful for me, to know what condition trigger the reconcile, we don't want unexpected reconcile, if we hide this information to Debug, it would be less easier to find out such problem, espically for un-experienced developers.
Besides, there is no log.Debug, instead, it's log.V(2).Info

Copy link
Collaborator

@irisdingbj irisdingbj Sep 3, 2024

Choose a reason for hiding this comment

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

We should add an option when controller start, to let user select the log level they want to set, eg: --logLevel debug
in such case, they can have much info in logs.
It does not make sense to have most info output to Info level by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should add an option when controller start, to let user select the log level they want to set, eg: --logLevel debug in such case, they can have much info in logs. It does not make sense to have most info output to Info level by default.

it's implemented

Copy link
Collaborator

@zhlsunshine zhlsunshine left a comment

Choose a reason for hiding this comment

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

LGTM

@irisdingbj
Copy link
Collaborator

Thanks @KfreeZ for adding this support, Can we change it a little bit to better relflect HTTP VERBs? like:

curl -X GET "http://localhost:8008/loglevel -- this will return current loglevel for user
curl -X PUT "http://localhost:8008/loglevel -d '{"log_level":"debug"}' -- this will change current loglevel to 'debug`
also plz update docs to tell user how to use it.

@KfreeZ
Copy link
Collaborator Author

KfreeZ commented Sep 11, 2024

Thanks @KfreeZ for adding this support, Can we change it a little bit to better relflect HTTP VERBs? like:

curl -X GET "http://localhost:8008/loglevel -- this will return current loglevel for user curl -X PUT "http://localhost:8008/loglevel -d '{"log_level":"debug"}' -- this will change current loglevel to 'debug` also plz update docs to tell user how to use it.

done

@irisdingbj irisdingbj merged commit a18404e into opea-project:main Sep 11, 2024
8 checks passed
@KfreeZ KfreeZ added this to the v1.0 milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants