HPCC-30403 Provide ECL based API for Tracing #18057

rpastrana · 2023-11-21T04:45:02Z

Declares several ecl calls to logging library
Defines getTraceSpanHeader, getTraceStateHeader
Defines getTraceID, getSpanID, setSpanAttribute

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:

Smoketest:

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

Testing:

github-actions · 2023-11-21T04:45:29Z

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

ghalliday

@rpastrana looks good. A few very minor comments.

ghalliday · 2023-11-21T16:21:40Z

system/jlib/jtrace.cpp

@@ -730,6 +730,18 @@ IProperties * getClientHeaders(const ISpan * span)
    return headers.getClear();
 }

+void setSpanAttribute(ISpan * span, const char * name, const char * val)


minor: Not sure why you would call this rather than span->setSpanAttribute()

yeah I ended up going this route because in logging.cpp, ctx->queryContextLogger() returns a const, this approach seemed to bypass the const restriction on the ctxLogger. Do we want to make the contexlogger non-const?

You can still call

activeSpan->setSpanAttribute(name, value);

from within a const function. const generally prevents direct member variables from being modified - it does not prevent objects pointed to by member variables from being modified.

ghalliday · 2023-11-21T16:27:22Z

plugins/logging/logging.cpp

+{
+    StringBuffer ret;
+    Owned<IProperties> httpHeaders = ctx->queryContextLogger().getSpanContext();
+    if (httpHeaders)


Can this ever return null? If not it is clearer without the if() test. (It also confuses static analysers).

ghalliday · 2023-11-21T16:29:21Z

plugins/logging/logging.cpp

+    StringBuffer attVal(valueLen, value);
+
+    ctx->queryContextLogger().setSpanAttribute(attName.str(), attVal.str());
+}


minor: newline missing from end of file.

ghalliday · 2023-11-21T16:33:39Z

roxie/ccd/ccdserver.cpp

+    }
+    virtual void setSpanAttribute(const char *name, const char *value) const override
+    {
+        return ctx->setSpanAttribute(name, value);


observation: This did not use to be legal (return with a void expression), but it appears it is now.

rpastrana · 2023-11-21T20:17:06Z

@ghalliday made these change during pto hoping to get the pr merged in time for latest rc.
However, this does need some unittests which I hope to include in follow up pr.
Any info on how ecl plugins are typically auto-tested? I'd like to add at least the following:

import lib_logging;

output(lib_logging.Logging.getSpanID());
output(lib_logging.Logging.getTraceID());

>379563f3fcd56bc9
>0c483675c6c887d9fb49885c0f2ba116

import lib_logging;

output(lib_logging.Logging.getTraceSpanHeader());
output(lib_logging.Logging.getTraceStateHeader());

>00-a9f8b7d14d25d9233b829b3481bfaf93-6846688c1431ef1d-01

import lib_logging;
output(lib_logging.Logging.getSpanID());
lib_logging.Logging.setSpanAttribute('some','{"lines":[{"fields":[{"timestamp": "2023-11-06T20:52:19.265Z","hpcc.log.threadid": "232","hpcc.log.timestamp": "2023-11-06 20:52:19.265","kubernetes.container.name": "myeclccserver",}]},{"fields":[{"hpcc.log.jobid": "W20231111-222222","hpcc.log.audience": "OPR","hpcc.log.message": "hthor build community_9.4.9-closedown0Debug[heads/HPCC-30401-JTrace-optional-traceid-support-0-gc85263-dirty]",          "kubernetes.pod.name": "hthor-job-w20231106-205213-jdcss",}]}]}');

ghalliday · 2023-11-22T10:19:26Z

system/jlib/jtrace.cpp

@@ -730,6 +730,18 @@ IProperties * getClientHeaders(const ISpan * span)
    return headers.getClear();
 }

+void setSpanAttribute(ISpan * span, const char * name, const char * val)


You can still call

activeSpan->setSpanAttribute(name, value);

from within a const function. const generally prevents direct member variables from being modified - it does not prevent objects pointed to by member variables from being modified.

ghalliday · 2023-11-22T10:21:27Z

plugins/logging/logging.cpp

-    if (httpHeaders)
-        ret.set(httpHeaders->queryProp("traceID"));
-
+    StringBuffer ret(ctx->queryContextLogger().getSpanContext()->queryProp("traceID"));


This change isn't correct - it will now leak the properties (we return pointers rather than smart pointers).

ghalliday · 2023-11-28T11:04:30Z

plugins/logging/logging.cpp

+    return ret.detach();
+}
+
+LOGGING_API void LOGGING_CALL setSpanAttribute(ICodeContext *ctx,


trivial. Is this newline accidental?

ghalliday

Please squash (one trivial comment about a newline)

ghalliday · 2023-11-28T11:05:46Z

@rpastrana Also looks like it needs rebasing.

rpastrana · 2023-11-28T17:35:15Z

@ghalliday added a regression suite test case for this, if nothing else, to document the use case. If you're ok with the change I'll squash.

ghalliday

All looks good, please squash

- Adds several tracing related calls to ECL logging lib - Defines getTraceSpanHeader, getTraceStateHeader - Defines getTraceID, getSpanID, setSpanAttribute - Adds regression tests Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexisrisk.com>

rpastrana · 2023-11-29T14:09:40Z

@ghalliday squashed

rpastrana changed the title ~~HPCC-30403 Provid ECL based API for Tracing~~ HPCC-30403 Provide ECL based API for Tracing Nov 21, 2023

rpastrana requested a review from ghalliday November 21, 2023 04:46

ghalliday reviewed Nov 21, 2023

View reviewed changes

rpastrana requested a review from ghalliday November 21, 2023 20:04

ghalliday requested changes Nov 22, 2023

View reviewed changes

rpastrana requested a review from ghalliday November 27, 2023 17:43

ghalliday reviewed Nov 28, 2023

View reviewed changes

ghalliday approved these changes Nov 28, 2023

View reviewed changes

rpastrana requested a review from ghalliday November 28, 2023 17:08

rpastrana force-pushed the HPCC-30403-ECL-Otel branch from f15f89b to 3a558fd Compare November 28, 2023 17:33

ghalliday approved these changes Nov 29, 2023

View reviewed changes

rpastrana force-pushed the HPCC-30403-ECL-Otel branch from 3a558fd to ef2ba93 Compare November 29, 2023 14:08

ghalliday merged commit 929d7fc into hpcc-systems:candidate-9.4.x Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30403 Provide ECL based API for Tracing #18057

HPCC-30403 Provide ECL based API for Tracing #18057

rpastrana commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023

ghalliday left a comment

ghalliday Nov 21, 2023

rpastrana Nov 21, 2023

ghalliday Nov 22, 2023

ghalliday Nov 21, 2023

ghalliday Nov 21, 2023

ghalliday Nov 21, 2023

rpastrana commented Nov 21, 2023

ghalliday Nov 22, 2023

ghalliday Nov 22, 2023

ghalliday Nov 28, 2023

ghalliday left a comment

ghalliday commented Nov 28, 2023

rpastrana commented Nov 28, 2023

ghalliday left a comment

rpastrana commented Nov 29, 2023

HPCC-30403 Provide ECL based API for Tracing #18057

HPCC-30403 Provide ECL based API for Tracing #18057

Conversation

rpastrana commented Nov 21, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 21, 2023

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday commented Nov 28, 2023

rpastrana commented Nov 28, 2023

ghalliday left a comment

Choose a reason for hiding this comment

rpastrana commented Nov 29, 2023

rpastrana commented Nov 21, 2023 •

edited

Loading