search

temporalio / temporal

Temporal service
https://docs.temporal.io
MIT License
11.95k stars 843 forks source link

Dump only the type of request when we fail to find Namespace info #5941

Closed tdeebswihart closed 5 months ago

tdeebswihart commented 5 months ago

What changed?

We now only log the type of the request we failed to find namespace details on rather than the contents of the request, which required expensive reflection.

Why?

This is an expensive error path otherwise. We discovered we spent more time in this error path than in our actual logic when running an experiment

How did you test it?

Potential risks

None

Documentation

Is hotfix candidate?

No

tdeebswihart commented 5 months ago

Do we need the request type at all? I.e. will this error be bubbled up somewhere visible or will it just get thrown away?

I'm not sure at the moment; I don't have the logs, only the CPU profile. I'll see if I can scrounge up the logs

tdeebswihart commented 5 months ago

Do we need the request type at all? I.e. will this error be bubbled up somewhere visible or will it just get thrown away?

...we don't have any logs because MustGetNamespaceName swallows the error

dnr commented 5 months ago

...we don't have any logs because MustGetNamespaceName swallows the error

In that case I'd say return a static error, don't even printf

tdeebswihart commented 5 months ago

...we don't have any logs because MustGetNamespaceName swallows the error

In that case I'd say return a static error, don't even printf

GetNamespaceName is called elsewhere too, just this particular usage didn't :)

dnr commented 5 months ago

GetNamespaceName is called elsewhere too, just this particular usage didn't :)

Ah. Fine either way then