chore: changing digest and hash log format from bytes to hex #2363
Conversation
|
You can find the image built from this PR at Built from 91a9f46 |
|
Seems to be working: |
I agree 100% that having the hash truncated is better/most comfortable. The issue is that this was requested so messages could be easily matched with Postgres errors such as and the hash isn't truncated here. So I think it's better to remain consistent and have the hash in the same format everywhere. @apentori what do you think? |
Actually it is requested to help status Desktop and Mobile team identify message relialbility issues. From what @chaitanyaprem told me,the Postgres error is like to the fleet architecture : 2 nodes trying to write on 1 db. I think it's better to have the full hash, specially for a Trace log that wont be usefull for a long time. |
waku/waku_archive/archive.nim
Outdated
| @@ -111,7 +111,7 @@ proc handleMessage*(w: WakuArchive, | |||
| msgReceivedTime = if msg.timestamp > 0: msg.timestamp | |||
| else: getNanosecondTime(getTime().toUnixFloat()) | |||
|
|
|||
| trace "handling message", pubsubTopic=pubsubTopic, contentTopic=msg.contentTopic, timestamp=msg.timestamp, digest=msgDigest, messageHash=msgHash | |||
| trace "handling message", pubsubTopic=pubsubTopic, contentTopic=msg.contentTopic, timestamp=msg.timestamp, digest=toHex(msgDigest.data), messageHash=toHex(msgHash) | |||
There was a problem hiding this comment.
Upon latest ligging research, can we just move any conversion/calculation away from logging line?
Until we don't know what cause double call of any proc appears in the log arguments.
It is also involves memory allocation for the hex string, I would avoid that.
There was a problem hiding this comment.
But this leads to another problem, if trace is disabled the hash would still be calculated which would reduce performance even in normal scenario.
Unless we can access and use the logging macro and put the hash calculation statement under it.
There was a problem hiding this comment.
I actually just discussed this point with Zoltán :))
In such cases we can check the log level and compute depending on it. I just added this fix without doing a specific check for log level, as I'm doing an extremely cheap computation - converting a constant size hash from bytes to hex.
For bigger computations, I guess the best thing is to compute it based on the log level. Although it seems not very elegant, not sure if there's a better way to abstract it.
Most important thing might be to figure out why computation happens twice when added as a log argument. The biggest problem here is the double memory allocation.
There was a problem hiding this comment.
Yeah exactly. Agreed to enter an issue for chronicles lib. That can solve this issue in an optimal way.
Within this in mind, I would agree to keep it if you think so. I would have like to claim the attention on hidden costs of such calls.
There was a problem hiding this comment.
That's a good approach, let's check it directly with Chronicles. In the meantime, I'll leave the computation outside the log. It's a super simple/cheap one, so if we can avoid having them inside then better
gabrielmer
force-pushed
the
chore-changing-message-hash-log-format
branch
from
0f39909 to
e80cd8b
Compare
Description
Changing
digestandmessageHashformats in log from bytes to hex for readability.Issue
closes #2362