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

feat: improve logging of reactive variable changes #517

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iisakkirotko
Copy link
Collaborator

@iisakkirotko iisakkirotko commented Feb 22, 2024

The way that the log_level-settings is set is kind of hacky right now, but we need to discuss implementing a real solution in a different way

Copy link
Collaborator Author

iisakkirotko commented Feb 22, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @iisakkirotko and the rest of your teammates on Graphite Graphite

@iisakkirotko iisakkirotko force-pushed the 02-22-feat_improve_logging_of_reactive_variable_changes branch from d632c1b to 348bf7a Compare February 22, 2024 14:53
@iisakkirotko
Copy link
Collaborator Author

iisakkirotko commented Feb 23, 2024

So testing the performance, the cost is only at start-up of the server, but with this change it takes about 4s longer to start the server than without. I'll take a look at what part of the change requires so much time next week.

P.S. It seems like reactive variables get initialized twice?

@iisakkirotko iisakkirotko force-pushed the 02-22-feat_improve_logging_of_reactive_variable_changes branch from 348bf7a to 8277e1c Compare February 26, 2024 14:32
@maartenbreddels
Copy link
Contributor

This PR led to a discussion about logging, for that I opened an issue #527

@maartenbreddels
Copy link
Contributor

maartenbreddels commented Feb 28, 2024

Btw, I expected this PR to build on top of #515 ? Maybe a squash issue?

TODO:

  • find out why it takes so much more time (Iisakki)
  • find out why it seems to initialize twice (Maarten)

@iisakkirotko
Copy link
Collaborator Author

Btw, I expected this PR to build on top of #515 ? Maybe a squash issue?

To me these changes feel unrelated (even though they both use inspect.stack()), since one is about logging, while the other is fixing an issue. You could also argue that #515 could be based on this one, and make use of var._varname.

@iisakkirotko
Copy link
Collaborator Author

The performance of various ways to get caller names was tested here, from which it's pretty fair to say that we chose the slowest way to do this initially. I'll take a look at using a rewriting this PR using one of the better performing methods.

@iisakkirotko iisakkirotko force-pushed the 02-22-feat_improve_logging_of_reactive_variable_changes branch 4 times, most recently from 3977e7d to 62f17b1 Compare March 8, 2024 13:26
@maartenbreddels maartenbreddels force-pushed the 02-22-feat_improve_logging_of_reactive_variable_changes branch 2 times, most recently from 0a3d202 to bcea747 Compare March 10, 2024 21:28
@iisakkirotko iisakkirotko force-pushed the 02-22-feat_improve_logging_of_reactive_variable_changes branch from bcea747 to 8395690 Compare March 26, 2024 08:32
Copy link

render bot commented Mar 26, 2024

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

Successfully merging this pull request may close these issues.

None yet

2 participants