Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HPCC-30795 Expand JTrace exporter support and configuration #18025

Merged
merged 1 commit into from
Nov 17, 2023
Merged

HPCC-30795 Expand JTrace exporter support and configuration #18025

merged 1 commit into from
Nov 17, 2023

Conversation

rpastrana
Copy link
Member

@rpastrana rpastrana commented Nov 14, 2023 •
edited
Loading

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:

@github-actions GitHub Actions
Copy link

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

@rpastrana rpastrana force-pushed the HPCC-30795-OTLP-Exporter-Configuration branch from ec5c416 to 658ba51 Compare November 14, 2023 03:51
@rpastrana rpastrana requested a review from ghalliday November 14, 2023 03:52
ghalliday
ghalliday reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good. A few relatively minor comments.

system/jlib/jtrace.cpp Outdated
trace_opts.url = url.str();
}
else
trace_opts.url = opentelemetry::exporter::otlp::GetOtlpDefaultHttpTracesEndpoint();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: This appears to be the default anyway.

system/jlib/jtrace.cpp Outdated
else if (stricmp(exportType.str(), "OTLP")==0 || stricmp(exportType.str(), "OTLP-HTTP")==0)
{
opentelemetry::exporter::otlp::OtlpHttpExporterOptions trace_opts;
if (exportConfig->hasProp("@url"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more common/simpler way of coding this, avoiding a string copy, would be

const char * url = exportConfig->queryProp("@url");
if (url)
   trace_opts.url = url;

Same for the endpoint code below.

system/jlib/jtrace.cpp Outdated
@@ -860,26 +890,28 @@ class CTraceManager : implements ITraceManager, public CInterface
{
#ifdef TRACECONFIGDEBUG
Owned<IPropertyTree> testTree;
if (!traceConfig)
//if (!traceConfig)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left in after debugging?

system/jlib/CMakeLists.txt Outdated
@@ -261,7 +261,7 @@ target_link_libraries ( jlib
# opentelemetry-cpp::otlp_grpc_log_record_exporter - Imported target of opentelemetry-cpp::otlp_grpc_log_record_exporter
# opentelemetry-cpp::otlp_grpc_metrics_exporter - Imported target of opentelemetry-cpp::otlp_grpc_metrics_exporter
# opentelemetry-cpp::otlp_http_client - Imported target of opentelemetry-cpp::otlp_http_client
# opentelemetry-cpp::otlp_http_exporter - Imported target of opentelemetry-cpp::otlp_http_exporter
opentelemetry-cpp::otlp_http_exporter # - Imported target of opentelemetry-cpp::otlp_http_exporter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trivial: imported no longer lines up. Was that the intention?

helm/examples/tracing/README.md Outdated
"Status":"Unset",
"TraceState":"hpcc=4b960b3e4647da3f",
"Attributes":{
"Caller-Id":"IncomingCID",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now hpcc.callerid etc.

helm/examples/tracing/README.md Outdated

Sample span reported as log event:
```console
000000A3 MON EVT 2023-10-10 22:12:23.827 24212 25115 Span start: {"Type":"Server","Name":"propegatedServerSpan","GlobalID":"IncomingUGID","CallerID":"IncomingCID","LocalID":"JDbF4xnv7LSWDV4Eug1SpJ","TraceID":"beca49ca8f3138a2842e5cf21402bfff","SpanID":"4b960b3e4647da3f"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

picky: propagated rather than propegated - and below.

helm/examples/tracing/README.md Outdated
helm install myTracedHPCC hpcc/hpcc -f otlp-http-collector-default.yaml
```
## Tracing information
HPCC tracing information includes data needed to trace requests as they traverse over distributed components and information related to sub-tasks actuated to process the request in the form of spans. Each trace and all its related spans are assigned unique IDs which follow the Open Telemetry standard.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to reword "information related to sub-tasks actuated to process the request in the form of spans". I found it hard to understand, especially actuated.

system/jlib/jtrace.cpp
StringBuffer endPoint;
exportConfig->getProp("@endpoint", endPoint);
opts.endpoint = endPoint.str();
}

opts.use_ssl_credentials = exportConfig->getPropBool("@useSslCredentials", false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth discussing with business units whether they are using a secure protocol. (I don't see any need.) If so we will need some jiras to look at supporting that with secrets etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% in agreement. I had started that conversation w/ Tony at some point, but that tailed off. I'll reach out again.

@rpastrana rpastrana force-pushed the HPCC-30795-OTLP-Exporter-Configuration branch from bda10a7 to c1979f1 Compare November 15, 2023 03:25
@rpastrana rpastrana requested a review from ghalliday November 15, 2023 03:25
ghalliday
ghalliday approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpastrana looks good. Please fix up the newline character in the readme, create a jira for the secure credentials, and squash.

helm/examples/tracing/README.md Outdated
- sslCredentialsCACcert
- endpoint: (default localhost:4317) The endpoint to export to. By default the OpenTelemetry Collector's default endpoint.
- useSslCredentials - By default when false, uses grpc::InsecureChannelCredentials; If true uses sslCredentialsCACcert
- sslCredentialsCACcert - String representation of .pem file to be used for SSL encryption.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't secure - we will need a different way of providing he cert (via a secret) if this option is actually going to be used. Probably best to remove this line and add support in a separate PR (please create a jira if you think it will be needed).
Alternatively add a comment that this format is deprecated and will be removed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://track.hpccsystems.com/browse/HPCC-30850

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively for this initial offering, does it make sense to support the .pem path option instead?
HPCC-30850 will support proper k8s/vault secret

helm/examples/tracing/README.md Outdated
},
"InstrumentedLibrary":"esp"

}```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still something strange about this file at this point... I think add a newline before the ``` characters (and delete line 90)

@rpastrana rpastrana requested a review from ghalliday November 15, 2023 14:11
@rpastrana
Copy link
Member Author

@ghalliday we can remove the OTLP-GRCP ssl support for now, but added the pem path option for your consideration.

@rpastrana rpastrana mentioned this pull request Nov 15, 2023
Merged
39 tasks
ghalliday
ghalliday approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpastrana please squash and create a jira for the secret (if not done already)

system/jlib/jtrace.cpp
exportConfig->getProp("@sslCredentialsCACcert", sslCACert);
opts.ssl_credentials_cacert_as_string = sslCACert.str();
StringBuffer sslCACertPath;
exportConfig->getProp("@sslCredentialsCACertPath", sslCACertPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good for merging this change, but there should still be a jira to use a secret if this option is going to be used.

@rpastrana rpastrana force-pushed the HPCC-30795-OTLP-Exporter-Configuration branch from ba7c055 to 331a918 Compare November 16, 2023 15:15
@rpastrana
HPCC-30795 Extends Jtrace exporter and configuration support
9128cfc 
- Adds Otel dependancy
- Exposes otlp-http exporter configuration
- Defines jtrace processor and exporter config syntax
- Adds support for OTLP HTTP/GRCP support
- Adds helm/tracing/readme documentation
- Adds sample values block for otlp exporters
- Adds ca cert path support

Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexisrisk.com>
@rpastrana rpastrana force-pushed the HPCC-30795-OTLP-Exporter-Configuration branch from 331a918 to 9128cfc Compare November 16, 2023 21:14
@rpastrana
Copy link
Member Author

@ghalliday rebased

@ghalliday ghalliday merged commit 7d694c6 into hpcc-systems:candidate-9.4.x Nov 17, 2023
41 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghalliday ghalliday ghalliday approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rpastrana @ghalliday