Skip to content

fix(metrics): protocol_state=None case not accounted for#223

Merged
jansegre merged 1 commit intodevfrom
fix/metrics-collection-error
Jun 7, 2021
Merged

fix(metrics): protocol_state=None case not accounted for#223
jansegre merged 1 commit intodevfrom
fix/metrics-collection-error

Conversation

@jansegre
Copy link
Member

@jansegre jansegre commented Jun 4, 2021

I recently got a traceback like this:

Traceback (most recent call last):
  File ".../python3.7/site-packages/twisted/internet/defer.py", line 311, in addCallbacks
    self._runCallbacks()
  File ".../python3.7/site-packages/twisted/internet/defer.py", line 654, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
  File ".../python3.7/site-packages/twisted/internet/base.py", line 447, in _continueFiring
    callable(*args, **kwargs)
  File ".../python3.7/site-packages/twisted/internet/base.py", line 706, in disconnectAll
    failure.Failure(main.CONNECTION_LOST))
--- <exception caught here> ---
  File ".../python3.7/site-packages/twisted/python/log.py", line 103, in callWithLogger
    return callWithContext({"system": lp}, func, *args, **kw)
  File ".../python3.7/site-packages/twisted/python/log.py", line 86, in callWithContext
    return context.call({ILogContext: newCtx}, func, *args, **kw)
  File ".../python3.7/site-packages/twisted/python/context.py", line 122, in callWithContext
    return self.currentContext().callWithContext(ctx, func, *args, **kw)
  File ".../python3.7/site-packages/twisted/python/context.py", line 85, in callWithContext
    return func(*args,**kw)
  File ".../python3.7/site-packages/twisted/internet/tcp.py", line 519, in connectionLost
    self._commonConnection.connectionLost(self, reason)
  File ".../python3.7/site-packages/twisted/internet/tcp.py", line 327, in connectionLost
    protocol.connectionLost(reason)
  File ".../python3.7/site-packages/twisted/internet/endpoints.py", line 146, in connectionLost
    return self._wrappedProtocol.connectionLost(reason)
  File ".../python3.7/site-packages/twisted/protocols/tls.py", line 403, in connectionLost
    ProtocolWrapper.connectionLost(self, reason)
  File ".../python3.7/site-packages/twisted/protocols/policies.py", line 125, in connectionLost
    self.wrappedProtocol.connectionLost(reason)
  File ".../hathor-core/hathor/p2p/protocol.py", line 357, in connectionLost
    self.on_disconnect(reason)
  File ".../hathor-core/hathor/p2p/protocol.py", line 275, in on_disconnect
    self.connections.on_peer_disconnect(self)
  File ".../hathor-core/hathor/p2p/manager.py", line 220, in on_peer_disconnect
    self.pubsub.publish(HathorEvents.NETWORK_PEER_DISCONNECTED, protocol=protocol)
  File ".../hathor-core/hathor/pubsub.py", line 166, in publish
    fn(key, args)
  File ".../hathor-core/hathor/metrics.py", line 219, in handle_publish
    if data['protocol'].state.state_name == HathorProtocol.PeerState.READY.name:
builtins.AttributeError: 'NoneType' object has no attribute 'state_name'

It might not be too important because that happened when exiting hathor-core, but it's still an error. In the future we should consider a better typing strategy on the data variable that metric events receive because currently it erases the types of its values (by effectively being a Dict[str, Any]), so mypy can't see these kind of errors.

@jansegre jansegre self-assigned this Jun 4, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2021

Codecov Report

Merging #223 (fd6e9b6) into dev (5e811b7) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #223      +/-   ##
==========================================
- Coverage   83.40%   83.38%   -0.03%     
==========================================
  Files         147      147              
  Lines       14107    14108       +1     
  Branches     1974     1974              
==========================================
- Hits        11766    11764       -2     
- Misses       1925     1927       +2     
- Partials      416      417       +1     
Impacted Files Coverage Δ
hathor/metrics.py 84.28% <50.00%> (+0.11%) ⬆️
hathor/p2p/manager.py 69.78% <0.00%> (-1.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e811b7...fd6e9b6. Read the comment docs.

@jansegre jansegre force-pushed the fix/metrics-collection-error branch from 83156ec to fd6e9b6 Compare June 4, 2021 21:04
@jansegre jansegre merged commit 21d9c71 into dev Jun 7, 2021
@jansegre jansegre deleted the fix/metrics-collection-error branch June 7, 2021 19:27
@jansegre jansegre mentioned this pull request Jun 23, 2021
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.

4 participants