HPCC-30301 JTrace log output improvements

[rpastrana]

• Sets output to JSON format
• Fixes derived destructor toString
• Ensures Parent span output is encapsulated
• Adds heads-up note on base destructor
• Adds cppunit test for json output

Type of change:

• This change is a bug fix (non-breaking change which fixes an issue).
• This change is a new feature (non-breaking change which adds functionality).
• This change improves the code (refactor or other change that does not change the functionality)
• This change fixes warnings (the fix does not alter the functionality or the generated code)
• This change is a breaking change (fix or feature that will cause existing behavior to change).
• This change alters the query API (existing queries will have to be recompiled)

Checklist:

• My code follows the code style of this project.
  • My code does not create any new warnings from compiler, build system, or lint.
• The commit message is properly formatted and free of typos.
  • The commit message title makes sense in a changelog, by itself.
  • The commit is signed.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly, or...
  • I have created a JIRA ticket to update the documentation.
  • Any new interfaces or exported functions are appropriately commented.
• I have read the CONTRIBUTORS document.
• The change has been fully tested:
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have checked that this change does not introduce memory leaks.
  • I have used Valgrind or similar tools to check for potential issues.
• I have given due consideration to all of the following potential concerns:
  • Scalability
  • Performance
  • Security
  • Thread-safety
  • Cloud-compatibility
  • Premature optimization
  • Existing deployed queries will not be broken
  • This change fixes the problem, not just the symptom
  • The target branch of this pull request is appropriate for such a change.
• There are no similar instances of the same problem that should be addressed
  • I have addressed them here
  • I have raised JIRA issues to address them separately
• This is a user interface / front-end modification
  • I have tested my changes in multiple modern browsers
  • The component(s) render as expected

Smoketest:

• Send notifications about my Pull Request position in Smoketest queue.
• Test my draft Pull Request.

Testing:

[rpastrana]

@ghalliday please review.
I decided to format jtrace output in json for programmatic consumption (I considered storing Ispan attributes as proptree and outputing to xml,json etc through jtree functionality but wanted to hear your thoughts on output format.

Also, you'll notice new derived class destructors which utilize derived class methods.

[github-actions]

https://track.hpccsystems.com/browse/HPCC-30301
Jira updated

[rpastrana]

Sample JTrace.toString output:

09:54:37.875281 30183 Span end: {"Type":"Server","Name":"propegatedServerSpan","HPCCGlobalID":"IncomingUGID","HPCCCallerID":"IncomingCID","SpanID":"bad8c01118f382c1","TraceID":"beca49ca8f3138a2842e5cf21402bfff","TraceFlags":"01","RemoteParentSpanID":"4b960b3e4647da3f"}

Json Pretty formatted:

{
  "Type": "Internal",
  "Name": "internalSpan2",
  "SpanID": "41808b19c7e998b5",
  "TraceID": "beca49ca8f3138a2842e5cf21402bfff",
  "TraceFlags": "01",
  "LocalParentSpan": {
    "Type": "Internal",
    "Name": "internalSpan",
    "SpanID": "51e4dcf79e2ef6c1",
    "TraceID": "beca49ca8f3138a2842e5cf21402bfff",
    "TraceFlags": "01",
    "LocalParentSpan": {
      "Type": "Client",
      "Name": "clientSpan",
      "SpanID": "249295ad39f881ef",
      "TraceID": "beca49ca8f3138a2842e5cf21402bfff",
      "TraceFlags": "01",
      "LocalParentSpan": {
        "Type": "Server",
        "Name": "propegatedServerSpan",
        "HPCCGlobalID": "IncomingUGID",
        "HPCCCallerID": "IncomingCID",
        "SpanID": "bad8c01118f382c1",
        "TraceID": "beca49ca8f3138a2842e5cf21402bfff",
        "TraceFlags": "01",
        "RemoteParentSpanID": "4b960b3e4647da3f"
      }
    }
  }
}

[ghalliday]

A few comments from out discussion - use beforeDispose() instead

[ghalliday]

Worth introducing a new toLog() which logs a subset of the information to avoid duplication.

[ghalliday]

Also a typo in the commit message

[ghalliday]

I'm not sure about the second commit. I think you have two different uses for toString() which have very different characteristics.

The first use - which the 1st commit implements well, is to dump out all the information about a span for debugging/programmer tracing.

The second use is to provide the information needed by an external tool (or developer) to process the logs and reconstruct the span structure. That is a different task. For instance:

I would only expect to output the parent spanid, not the grandparents (they can be gathered from earlier log entries) - so it wouldn't nest/recurse.
LocalParentSpan is an internal concept so shouldn't really be in something for external consumption. I would expect "ParentSpan:" on all items which was either an internal span id, or the remote span id for a server span.

That's why I would introduce a different function, rather than overloading the toString() function.

[rpastrana]

I'm not sure about the second commit. I think you have two different uses for toString() which have very different characteristics.

The first use - which the 1st commit implements well, is to dump out all the information about a span for debugging/programmer tracing.

The second use is to provide the information needed by an external tool (or developer) to process the logs and reconstruct the span structure. That is a different task. For instance:

I would only expect to output the parent spanid, not the grandparents (they can be gathered from earlier log entries) - so it wouldn't nest/recurse. LocalParentSpan is an internal concept so shouldn't really be in something for external consumption. I would expect "ParentSpan:" on all items which was either an internal span id, or the remote span id for a server span.

That's why I would introduce a different function, rather than overloading the toString() function.

ok, that is all very reasonable, not exactly in line with my takeaway from our offline conversation on this subject, but I'll go ahead and make the changes to meet these expectations.

[rpastrana]

@ghalliday please review.
Please note, I took great care to adhere to your written expectations. I am not a fan of keeping very similar toLog and toString sets of logic. Also, Overloading the parentSpanID might seem cleaner, but ultimately will cause confusion even if the parent type can be inferred from the span type.

[rpastrana]

internalSpan2->toLog

{
  "Type": "Internal",
  "Name": "internalSpan2",
  "SpanID": "fe439419be47653a",
  "TraceID": "beca49ca8f3138a2842e5cf21402bfff",
  "ParentSpanID": "6ab77ef196bcdb11"
}

internalSpan2->toString

{
  "Type": "Internal",
  "Name": "internalSpan2",
  "SpanID": "fe439419be47653a",
  "TraceID": "beca49ca8f3138a2842e5cf21402bfff",
  "TraceFlags": "01",
  "ParentSpan": {
    "Type": "Internal",
    "Name": "internalSpan",
    "SpanID": "6ab77ef196bcdb11"
  }
}

[ghalliday]

@ghalliday please review. Please note, I took great care to adhere to your written expectations. I am not a fan of keeping very similar toLog and toString sets of logic. Also, Overloading the parentSpanID might seem cleaner, but ultimately will cause confusion even if the parent type can be inferred from the span type.

On reflection, I'm equally happy with a ParentSpanId RemoteSpanId split. The main concern is that it would not recursively expand all the local parents.

I'll merge as-is, but I expect to see more changes in this area. For example I think you only need to store globalid/traceid in the server span - which will change what is logged and where (see my second PR for some changes in that direction).

[ghalliday]

@rpastrana please squash and I will merge. I'll then rebase my changes on top of it.

[rpastrana]

@ghalliday squashed