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

Add logger specific to API (#2086) #2100

Closed

Conversation

hoenirvili
Copy link
Contributor

@hoenirvili hoenirvili commented Aug 2, 2020

Motivation

All the grpc common functionality are placed in a single package and their all using the global logger. This is not good because what if the client want's to use a custom logger or other miscellaneous options?

Closes #2086

Changes

I've changed all files under api/grpcserver/* to make all the concrete implementation be flexible. By flexible I mean, using the option pattern and make all implementations support adding required miscellaneous things like support custom loggin, support grpc protocol options etc. Because I didn't wanted types like optionNode, optionMesh, etc. I've moved every concrete implementation into it's own package under grpcserver/* to make the naming once again sane and be more maintainable.

Test Plan

I've just re-run the unit tests and made sure that everything passes alongside with whole compilation of the main binary (on linux 64bit)

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed

lrettig added 4 commits Jul 30, 2020
Simplify the wrapped Log type
Set level of derived logger correctly, and warn if the level is too low
@hoenirvili hoenirvili force-pushed the add-logger-issue-2086 branch 3 times, most recently from 0f6edf9 to a3cbaf9 Compare Aug 2, 2020
@y0sher
Copy link
Contributor

y0sher commented Aug 2, 2020

Hi @hoenirvili , the whole thing looks great. I would really like @lrettig to have a look since he is working on the API atm.
also one thing I can tell is that this PR is missing some functionality we have for loggers - controlling the log level dynamically.
if you look on the top of the node.go flie you'd find constants we use to name each log, this is created by maintaining a map with the state and loading it from config or changed from the grpc api. please have a look and try to add it, contact me if you need any help.

@noamnelke noamnelke requested a review from lrettig Aug 2, 2020
Copy link
Member

@noamnelke noamnelke left a comment

some of the file renames were correctly understood by git and others weren't - can you please fix this so it's easier to see the changes in the moved files, and to maintain the correct history of the files?

cmd/node/node.go Show resolved Hide resolved
@noamnelke
Copy link
Member

noamnelke commented Aug 2, 2020

Thinking about this a bit more, I'm not sure the relatively big refactor and partition into packages should be in the same PR as the logger change. @lrettig ?

@noamnelke
Copy link
Member

noamnelke commented Aug 2, 2020

Note that TestSpacemeshApp_GrpcService panics.

@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 2, 2020

Hi @hoenirvili , the whole thing looks great. I would really like @lrettig to have a look since he is working on the API atm.
also one thing I can tell is that this PR is missing some functionality we have for loggers - controlling the log level dynamically.
if you look on the top of the node.go flie you'd find constants we use to name each log, this is created by maintaining a map with the state and loading it from config or changed from the grpc api. please have a look and try to add it, contact me if you need any help.

Ok, I think I understood what you mean. I did checked the code and saw the map of loggers and I saw that this is configurable and you can set dynamically the log level across multiple resources so I think I managed to add support/port the functionality to all services/servers types.

Can you check one more time and let me know if this is what you want?

cmd/node/node.go Outdated Show resolved Hide resolved
cmd/node/node.go Outdated Show resolved Hide resolved
@y0sher
Copy link
Contributor

y0sher commented Aug 3, 2020

Hi @hoenirvili , the whole thing looks great. I would really like @lrettig to have a look since he is working on the API atm.
also one thing I can tell is that this PR is missing some functionality we have for loggers - controlling the log level dynamically.
if you look on the top of the node.go flie you'd find constants we use to name each log, this is created by maintaining a map with the state and loading it from config or changed from the grpc api. please have a look and try to add it, contact me if you need any help.

Ok, I think I understood what you mean. I did checked the code and saw the map of loggers and I saw that this is configurable and you can set dynamically the log level across multiple resources so I think I managed to add support/port the functionality to all services/servers types.

Can you check one more time and let me know if this is what you want?

Exactly what I wanted, added a few minor comments. still waiting for @lrettig to take a look here before merging.

@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 3, 2020

Exactly what I wanted, added a few minor comments. still waiting for @lrettig to take a look here before merging.

I addressed the comments, how is it now?

@hoenirvili hoenirvili requested review from y0sher and noamnelke Aug 3, 2020
@y0sher
Copy link
Contributor

y0sher commented Aug 3, 2020

Exactly what I wanted, added a few minor comments. still waiting for @lrettig to take a look here before merging.

I addressed the comments, how is it now?

The logger attached to app is already named and is used for app logs. I'd rather we pass down the root logger created in the function that calls startAPIServices.
also sorry I missed it earlier but for the log level config to work you will also have to add config entries for your loggers and add them in addLogger.

@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 3, 2020

Exactly what I wanted, added a few minor comments. still waiting for @lrettig to take a look here before merging.

I addressed the comments, how is it now?

The logger attached to app is already named and is used for app logs. I'd rather we pass down the root logger created in the function that calls startAPIServices.
also sorry I missed it earlier but for the log level config to work you will also have to add config entries for your loggers and add them in addLogger.

Thanks once again for the patience, I just modified the code. How is it now?

Btw, I have a proposal, instead of relaying on that switch stmt why won't we just have a map of these keys like map[keys]bool or map[keys]struct{} and if an element is found then it will return ok and we can go and use the logger. This way we only have to modify in one place the map and everything will work and also we also have the benefit at runtime to add dynamic support for future keys.

Do you want me in this PR to add this or make a new one after this is merged and continue further improving this ?

@hoenirvili hoenirvili force-pushed the add-logger-issue-2086 branch 2 times, most recently from c0ecb8b to 6758412 Compare Aug 3, 2020
Copy link
Member

@lrettig lrettig left a comment

Thanks for the PR! Added a few quick comments. GitHub's diff looks off for the renamed files, I will take a closer look at those later. Could you rebase against the api-phase9 branch? We'll be merging that branch soon, and this will prevent some ugly merge conflicts when that happens. #2092 is also in flight and will land soon and might cause some conflicts here, so we might want to merge that in here as well to prevent downstream conflicts.

api/pb/api.proto Outdated Show resolved Hide resolved
cmd/node/node.go Outdated Show resolved Hide resolved
cmd/node/node.go Show resolved Hide resolved
@lrettig
Copy link
Member

lrettig commented Aug 3, 2020

Thinking about this a bit more, I'm not sure the relatively big refactor and partition into packages should be in the same PR as the logger change. @lrettig ?

Agree here, it would be nice to split up these concerns - the partitioning into packages isn't required for the logging is it?

@lrettig
Copy link
Member

lrettig commented Aug 3, 2020

some of the file renames were correctly understood by git and others weren't - can you please fix this so it's easier to see the changes in the moved files, and to maintain the correct history of the files?

I'm seeing this issue now as well, odd - @hoenirvili is this something you might be able to fix? I'm not sure why GH does this.

@y0sher
Copy link
Contributor

y0sher commented Aug 4, 2020

Exactly what I wanted, added a few minor comments. still waiting for @lrettig to take a look here before merging.

I addressed the comments, how is it now?

The logger attached to app is already named and is used for app logs. I'd rather we pass down the root logger created in the function that calls startAPIServices.
also sorry I missed it earlier but for the log level config to work you will also have to add config entries for your loggers and add them in addLogger.

Thanks once again for the patience, I just modified the code. How is it now?

Btw, I have a proposal, instead of relaying on that switch stmt why won't we just have a map of these keys like map[keys]bool or map[keys]struct{} and if an element is found then it will return ok and we can go and use the logger. This way we only have to modify in one place the map and everything will work and also we also have the benefit at runtime to add dynamic support for future keys.

Do you want me in this PR to add this or make a new one after this is merged and continue further improving this ?

Good job on fixing it. the only part missing are the entries in the config.toml file for future reference.
It would be great if you could try and improve this feature but I believe it is better to do so in a separate PR.

@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 4, 2020

Exactly what I wanted, added a few minor comments. still waiting for @lrettig to take a look here before merging.

I addressed the comments, how is it now?

The logger attached to app is already named and is used for app logs. I'd rather we pass down the root logger created in the function that calls startAPIServices.
also sorry I missed it earlier but for the log level config to work you will also have to add config entries for your loggers and add them in addLogger.

Thanks once again for the patience, I just modified the code. How is it now?
Btw, I have a proposal, instead of relaying on that switch stmt why won't we just have a map of these keys like map[keys]bool or map[keys]struct{} and if an element is found then it will return ok and we can go and use the logger. This way we only have to modify in one place the map and everything will work and also we also have the benefit at runtime to add dynamic support for future keys.
Do you want me in this PR to add this or make a new one after this is merged and continue further improving this ?

Good job on fixing it. the only part missing are the entries in the config.toml file for future reference.
It would be great if you could try and improve this feature but I believe it is better to do so in a separate PR.

I just did that now, I still need to figure out how to sync with api-phase9 and it should be fine after that I think.

@lrettig
Copy link
Member

lrettig commented Aug 4, 2020

I just did that now, I still need to figure out how to sync with api-phase9 and it should be fine after that I think.

Step one: create a backup of your current branch in case the rebase goes wrong :)

> git checkout -b mybranch-backup && git checkout -

Step two: rebase against api-phase9. Hopefully this is relatively painless.

> git rebase origin/api-phase9

Step three: force push your changes

> git push -f

Step four: change the base branch of this PR.

Some more possibly helpful info here:

@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 4, 2020

I just did that now, I still need to figure out how to sync with api-phase9 and it should be fine after that I think.

Step one: create a backup of your current branch in case the rebase goes wrong :)

> git checkout -b mybranch-backup && git checkout -

Step two: rebase against api-phase9. Hopefully this is relatively painless.

> git rebase origin/api-phase9

Step three: force push your changes

> git push -f

Step four: change the base branch of this PR.

Some more possibly helpful info here:

* https://www.digitalocean.com/community/tutorials/how-to-rebase-and-update-a-pull-request

* https://anavarre.net/how-to-rebase-a-github-pull-request/

* https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

Thanks man you're awesome, tho' I knew what I had to do (as in git commands), just I'm lazy to resolve all the conflicts that appeared after rebase. I will fix this asap tho.'

@hoenirvili
Copy link
Contributor Author

hoenirvili commented Aug 18, 2020

What about now? Is the patch fine?

jack17529 and others added 13 commits Aug 26, 2020
Add `/devtools` to `.dockerignore`  
Closes spacemeshos#2121


Co-authored-by: Lane Rettig <lanerettig@gmail.com>
…os#2125)

## Motivation
When udp message from a new peer would arrive and the udp session state cache was full we try to evict one session. if one of the sessions in the state is older than 24hours we remove it first. this code path was never tested and had called `Close` on the connection after removing it from the map.

Closes #
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes

added test and fixed.

## Test Plan
<!-- Please specify how these changes were tested 
(e.g. unit tests, manual testing, etc.) -->

## TODO
<!-- This section should be removed when all items are complete -->
- [x] Explain motivation or link existing issue(s)
- [x] Test changes and document test plan
- [x] Update documentation as needed
## Motivation
Closes spacemeshos#2126 

## Changes
- Fail fast when generating and sending transactions to new test accounts fails
- Pause after sending each tx to give the network a chance to gossip the tx before sending another from the same source account

## Test Plan
N/A, this is an update to tests
It's redundant and doesn't do anything
Make some data items in log configurable for tests
Test all three levels of loggers
Final test is not passing as setting dynamic log level is not currently
working
Removes this endpoint from the (old) GRPC API and removes the
corresponding method from App. Remove corresponding interface from API
and don't pass App object into API.

Update app tests accordingly. Add a method to log to allow fully custom
logs to be constructed.
Remove unused data file options, replace New with NewDefault
This functionality was removed
@lrettig lrettig added the log label Aug 27, 2020
@hoenirvili
Copy link
Contributor Author

hoenirvili commented Sep 9, 2020

So what's the right decision, I noticed that api-phase9 is merged with this branch. Should I point now the PR to api-phase9and when api-phase9 is ready it will be merged to master ?

Or do you want me this split this into multiple PR's and start sending one at a time to develop ?

@lrettig
Copy link
Member

lrettig commented Sep 10, 2020

It looks like this was closed automatically since the base branch was merged into develop. I can't reopen this. We'll need to transfer this over to a new PR against develop.

@lrettig
Copy link
Member

lrettig commented Sep 10, 2020

Or do you want me this split this into multiple PR's and start sending one at a time to develop ?

Yes let's do this! :)

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.

Add custom logger for GRPC API
5 participants