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.

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

[golang] Migrate Parametric app from grpc to http #3332

Merged
merged 22 commits into from
Nov 4, 2024

Conversation

mtoffl01
Copy link
Contributor

@mtoffl01 mtoffl01 commented Oct 28, 2024

Motivation

GRPC is hard to use; HTTP is easier to understand and maintain.

Changes

Migrates the web server used for golang parametric apps from grpc to net-http
Also, modifies the custom struct types used for decoding and manipulating json request data in the golang parametric app.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@mtoffl01 mtoffl01 marked this pull request as ready for review October 31, 2024 19:32
@mtoffl01 mtoffl01 requested review from mabdinur and a team as code owners October 31, 2024 19:32
@mtoffl01
Copy link
Contributor Author

mtoffl01 commented Nov 1, 2024

Note: The 1 failing golang test is unrelated to these changes and should be fixed by this PR in dd-trace-go: DataDog/dd-trace-go#2960

@@ -775,10 +775,8 @@ def test_tracestate_w3c_p_inject(self, test_agent, test_library):
with test_library:
with test_library.start_span(name="new_span") as span:
headers = test_library.inject_headers(span.span_id)

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just revert changes to this file?

@@ -162,9 +162,9 @@ def test_distinct_aggregationkeys_TS003(self, library_env, test_agent, test_libr

if test_server.lang == "golang":
test_library.flush()

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert changes to this one too?

@@ -1,4 +1,6 @@
def make_single_request_and_get_inject_headers(test_library, headers):
with test_library.start_span(name="name", service="service", resource="resource", http_headers=headers,) as span:
headers = test_library.inject_headers(span.span_id)
if not headers: # Check if headers is empty
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this causing errors when the Go server didn't have any headers to inject? Rather than this, could the Go server just return an empty dictionary like other implementations?

Copy link
Contributor

@ZStriker19 ZStriker19 left a comment

Choose a reason for hiding this comment

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

Just a few nits but looks good! Great job! 🥇

@mtoffl01 mtoffl01 merged commit 91f13cc into main Nov 4, 2024
36 checks passed
@mtoffl01 mtoffl01 deleted the mtoff/migrate-golang-http branch November 4, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants