Improve internal error handling

[nanoandrew4]

Here is a first draft of the ideas we were discussing the other day @jum
I thought it might be better to generate a whole new log line instead of appending extra fields, that way it is easier to filter them out if necessary, and they can have their own custom severity.

Here is what the JSON would look like:

{
  "insertId": "uhdtmf3jtufa",
  "jsonPayload": {
    "ngcplogs-error": {
      "msg": "sample internal error message",
      "line": -1,
      "file": "ngcplogger"
    },
    "container": {
      "name": "/epic_swanson",
      "imageName": "sample app",
      "created": "2024-05-27T20:28:29.21066164Z",
      "imageId": "sha256:c044a522fe290e497107e52b92d746b4d2a4593a9e868912054175c609gggggg",
      "id": "7a79bdbaec8974ab80d1cad5dc491ceb62ea6855b9c3e101817fd2b8984f5c3a"
    },
    "time": "2024-05-27T20:28:33.552853362Z",
    "instance": null
  },
  "resource": {
    "type": "project",
    "labels": {
      "project_id": "voltaic-wall-123456"
    }
  },
  "timestamp": "2024-05-27T20:28:33.553073117Z",
  "severity": "WARNING",
  "logName": "projects/voltaic-wall-123456/logs/ngcplogs-docker-driver",
  "receiveTimestamp": "2024-05-27T20:28:33.607406011Z"
}

Let me know what you think and what changes you would make, if any.

[jum]

Sorry, have been traveling since Sunday afternoon, will have some time to look into it in about two days. Am 29.05.2024 um 00:41 schrieb Andres ***@***.***>: @nanoandrew4 requested your review on: #12 Improve internal error handling. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because your review was requested.Message ID: ***@***.***>

[jum]

First I have to apologize for my delayed response, my fault.

I do have an issue with inclusion of the initial pull request for the change in this change set, it will probably require a force push to commit which will invalidate a bug fix to the original pull request for a bug fix (a missing nil check). But if it is easier; I can also rebase on my end.

Otherwise I am somewhat uneasy with the passing of the driverErr result by reference. It is definitely the easiest way to implement this quickly, that is for sure. But for a more clearer flow of information I would prefer to have that as methods on nGCPLogger and change any function signatures as methods on nGCPLogger to be able to access that state. What do you think?

[nanoandrew4]

No worries on the delayed response, this isn't our job, we do it because we want to, and we have things to do outside of this :)

Regarding the issue with the conflict, we will merge your changes first, and I will update this branch to avoid conflicts.

Regarding the return by reference, you mean that you think that creating a member variable in nGCPLogger and retrieving it from the driver via a new method, instead of returning it as an error, would be clearer? What is it about the approach that makes you uneasy? Returning an error from a method call seems pretty clear to me, since you are reporting that something went wrong during the execution of the method, and the caller has to handle the error. Or is it the code inside the nGCPLogger where you sense a lack of clarity due to how the error is handled internally, before it is returned?

[jum]

I am more or less uneasy about the fact that it is special error handling that could be generalized. If nGCPLogger had a method for recording operational errors and the extraction functions would be methods on nGCPLogger they could simply use that. Any other methods on nGCPLogger could use that interface as well.

It may be overkill though on a second thought, but I am just using one particular part of the development universe, (caddy and lots of go apps), but other tools might need other log extraction helpers.

I am thinking about how hugely helpful distributed logging and tracing is, and with GCP logging it is comparably easy to use compared to the elephant in the room, OTEL. Should be more widely known...

[nanoandrew4]

I think I see where you concern comes from, since the driverErr I pass around is not part of the nGCPLogger and is just a wrapper which is allocated on each log call. I do agree that the solution is not particularly elegant, and a more correct go approach would be more verbose, involving raw errors and handling them with a lot of if err != nil, which is also not ideal. Your approach would save some memory allocations and remove a couple lines of code.

However, with where the project is at right now, I think the generalized approach you outlined is a bit overkill as you said and less explicit, as it would involve checking a getter after each log request to see if something failed, whereas the approach I have in this PR returns the error directly and if you don't handle it explicitly, the linter/IDE will give you a warning, so you don't have to be familiar with the code base to understand the flow of the program. It also makes the logger less stateful, which can make it easier to get performance out of concurrent processing (if for example we wanted to spawn a goroutine for sending each log to GCP), and will reduce the chance of state related bugs entering the codebase, which can be a nightmare to debug. So although in the future we might end up doing what you suggest naturally, I would prefer that generalizations are done when needed and they provide a real advantage now, and keep things as simple as possible so they are easier to maintain.

If you have any other feedback I'd be happy to hear it, or discuss it over a PR :)