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-30389 Split JTrace getSpanContext function #18161

Merged
merged 1 commit into from
Jan 19, 2024
Merged

HPCC-30389 Split JTrace getSpanContext function #18161

merged 1 commit into from
Jan 19, 2024

Conversation

rpastrana
Copy link
Member

@rpastrana rpastrana commented Dec 20, 2023 •
edited
Loading

  • Adjusts ISpan getSpanContex signature
  • Adds new ISpan getClientHeaders function

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 rpastrana requested a review from ghalliday December 20, 2023 18:10
@github-actions GitHub Actions
Copy link

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

ghalliday
ghalliday reviewed Jan 3, 2024
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 like a good change - the new functions are more logical.
One minor comment and one question.
Also a couple of trivial issues with the commit message that are probably worth addressing at the same time.

  1. Splits should probably be "Split" to aid readability.
  2. "getClientHeaders funtion" should be "function"

system/jlib/jtrace.cpp Outdated
{
if (ctxProps == nullptr)
return false;
if (clientHeaders == nullptr)
Copy link
Member

Choose a reason for hiding this comment

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

minor: I don't think it is ever useful to call this if clientHeaders is null, so better to assert (e.g. assertex(clientHeaders)). Otherwise the derived functions also need to check that clientHeaders also non-null for consistency.

same goes for getSpanContext()

testing/unittests/jlibtests.cpp Outdated

CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected getSpanContext failure detected", true, getSpanCtxSuccess);
//CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected getClientHeaders failure detected", true, retrievedClientHeaders->length()>0);
Copy link
Member

Choose a reason for hiding this comment

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

Should this test be uncommented again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to ensure at least the parentspan was provided.

@rpastrana rpastrana force-pushed the HPCC-30389-SplitISpan-getSpanContext branch from d81b305 to faf9716 Compare January 8, 2024 17:31
@rpastrana rpastrana changed the title HPCC-30389 Splits JTrace getSpanContext function HPCC-30389 Split JTrace getSpanContext function Jan 8, 2024
@rpastrana rpastrana requested a review from ghalliday January 8, 2024 17:34
@rpastrana rpastrana force-pushed the HPCC-30389-SplitISpan-getSpanContext branch from faf9716 to 1ab6489 Compare January 9, 2024 13:24
@rpastrana
Copy link
Member Author

@GordonSmith I'm noticing vcpkg failures in this PR related to opentelemetry download failures:
https://github.com/hpcc-systems/HPCC-Platform/actions/runs/7461687341/job/20302370351?pr=18161#step:10:54
can you help make sense of the problem?

@rpastrana
Copy link
Member Author

@ghalliday made the minor changes requested and ended up squashing

@GordonSmith
Copy link
Member

@rpastrana That url is working fine here - I will wait for your re-run to fail before I look any further?

@rpastrana
Copy link
Member Author

@rpastrana That url is working fine here - I will wait for your re-run to fail before I look any further?

Thanks @GordonSmith , looks like the latest checks are running fine now.

@rpastrana
Copy link
Member Author

@ghalliday added a few minor unittest cleanup changes

@rpastrana rpastrana requested a review from ghalliday January 10, 2024 21:14
ghalliday
ghalliday approved these changes Jan 11, 2024
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

@rpastrana
HPCC-30389 Split JTrace getSpanContext function
7b68ed7 
- Adjusts ISpan getSpanContex signature
- Adds new ISpan getClientHeaders function
- Ensures tracestate is produced

Signed-off-by: Rodrigo Pastrana <[email protected]>
@rpastrana rpastrana force-pushed the HPCC-30389-SplitISpan-getSpanContext branch from 5a1a49a to 7b68ed7 Compare January 11, 2024 17:04
@rpastrana
Copy link
Member Author

@rpastrana please squash

Done.

@ghalliday ghalliday merged commit 4b4122b into hpcc-systems:candidate-9.4.x Jan 19, 2024
43 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.
3 participants
@rpastrana @GordonSmith @ghalliday