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

Fix up pusher logging a bit #4716

Merged
merged 5 commits into from Feb 22, 2019

Conversation

Projects
None yet
3 participants
@erikjohnston
Copy link
Member

erikjohnston commented Feb 22, 2019

No description provided.

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Feb 22, 2019

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Feb 22, 2019

(attempts to) fix #4710 I guess

@@ -97,6 +97,11 @@ def __init__(self, hs, pusherdict):
pusherdict['pushkey'],
)

if self.data is None:
raise PusherConfigException(

This comment has been minimized.

@richvdh

richvdh Feb 22, 2019

Member

will we actually log any less for a PusherConfigException than for a TypeError or whatever it was?

This comment has been minimized.

@erikjohnston

erikjohnston Feb 22, 2019

Author Member

Woops, I missed a code path where it wasn't handled.

@@ -56,7 +56,7 @@ def create_pusher(self, pusherdict):
f = self.pusher_types.get(kind, None)
if not f:
return None
logger.info("creating %s pusher for %r", kind, pusherdict)
logger.debug("creating %s pusher for %r", kind, pusherdict)

This comment has been minimized.

@richvdh

richvdh Feb 22, 2019

Member

I'm not entirely convinced by this. it's the only thing that gets logged during minutes of cpu-spinning, and it's not massively high-volume.

This comment has been minimized.

@erikjohnston

erikjohnston Feb 22, 2019

Author Member

It's logging several times a millisecond with a reasonably a non-trivial dict, which feels a bit OTT. There are ~170K pushers, with a log length of ~600 characters, means about 102MB of logs

I could make it so it logs in batches the progress?

This comment has been minimized.

@richvdh

richvdh Feb 22, 2019

Member

fair enough. let's see how it looks.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #4716 into develop will decrease coverage by 0.11%.
The diff coverage is 33.33%.

@@             Coverage Diff             @@
##           develop    #4716      +/-   ##
===========================================
- Coverage    75.14%   75.03%   -0.12%     
===========================================
  Files          340      340              
  Lines        34815    34822       +7     
  Branches      5702     5704       +2     
===========================================
- Hits         26162    26127      -35     
- Misses        7044     7074      +30     
- Partials      1609     1621      +12
@richvdh
Copy link
Member

richvdh left a comment

lgtmish

pusherdict.get('user_name'),
pusherdict.get('app_id'),
pusherdict.get('pushkey'),
e,

This comment has been minimized.

@richvdh

richvdh Feb 22, 2019

Member

I think you generally need the type of the exception as well as the stringification

(mostly because KeyErrors are otherwise extremely confusing:

>>> try:
...     {}['a']
... except Exception as e:
...     print(e)
...
'a'

)

This comment has been minimized.

@erikjohnston

erikjohnston Feb 22, 2019

Author Member

Err, we know the type since we're only catching the one PusherConfigException exception?

@@ -56,7 +56,7 @@ def create_pusher(self, pusherdict):
f = self.pusher_types.get(kind, None)
if not f:
return None
logger.info("creating %s pusher for %r", kind, pusherdict)
logger.debug("creating %s pusher for %r", kind, pusherdict)

This comment has been minimized.

@richvdh

richvdh Feb 22, 2019

Member

fair enough. let's see how it looks.

@erikjohnston erikjohnston merged commit d14e94b into develop Feb 22, 2019

0 of 5 checks passed

ci/circleci: sytestpy2merged CircleCI is running your tests
Details
ci/circleci: sytestpy2postgresmerged CircleCI is running your tests
Details
ci/circleci: sytestpy3merged CircleCI is running your tests
Details
ci/circleci: sytestpy3postgresmerged CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@erikjohnston erikjohnston deleted the erikj/pusher_logging branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.