vmware / splinterdb

High Performance Embedded Key-Value Store
https://splinterdb.org
Apache License 2.0
680 stars 57 forks source link

(#505) Enhance platform_error_log() to prepend source code & OS-pid/tid-info #506

Closed gapisback closed 1 year ago

gapisback commented 1 year ago

This commit enhances platform_error_log() to prepend source-code location triplet (file, line, function-name) and OS-pid/tid and thread-ID to the message being printed. This extra info makes it easier to quickly find out where-from the code-base a particular error is raised and affecting which OS process / Splinter thread.


NOTE: This was a diagnostic enhancement I had used during on-going prototype integration code. I found it very useful to have this extra-info printed for an error.

Rather than applying these few extra fields to every call to platform_error_log() that wants to gain this new functionality, I think it will be better to do this generically. That way, in all instances of an error message we will get this extra diagnostic info reliably.

netlify[bot] commented 1 year ago

Deploy Preview for splinterdb canceled.

Name Link
Latest commit fac0e010b2f13c1b3fd232f05ca1c542282559b1
Latest deploy log https://app.netlify.com/sites/splinterdb/deploys/63a888981d0b810009ab515a
gapisback commented 1 year ago

Hi, @ajhconway : Let me think about your suggestion here.

I would have liked this to be the default option. Opting-in (for now) is still an option, leaving open the possibility to make this output the default, once we get more experience with the usefulness of this kind of behaviour in future.

Let me also talk to @rosenhouse when he returns next week. We had doodled around some ideas of, perhaps, making the newly added VERBOSE=1 env-var, that is used to spit out messages from unit-tests, a --cmd-line flag.

I am wondering if it's a good time to reconcile both efforts and may be add cmd-line flags like --verbose-output (for the env-var) and (something like) --extended-error-info for this new behaviour.

(The term Extended Error Info generally refers to this kind of extra info printed with error messages, listing stuff like hostname, ip-addresses and so on. Here's an example.)

@rtjohnso may have some thoughts / opinions on the suggested name of --extended-error-info.

If this feature has to be made opt-in, I would like to find a way to make it so that it can only be enabled for a specific thread, so that one can toggle on / off this extra behaviour only for certain processes / threads that may be running into a specific error situation that one is debugging. Let me think about that ...

gapisback commented 1 year ago

@rtjohnso : Consider cmd line option of --error-level to future-proof this.

gapisback commented 1 year ago

Based on review feedback (see note from Alex) and internal discussion w/ core team during PR reviews, the recommendation is to not pursue with this change.

See the notes in issue #505, explaining why the decision was made to not pursue this change-set.