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

Allow user-provided User-Agent request header #1272

Merged
merged 3 commits into from
Oct 28, 2024
Merged

Conversation

polRk
Copy link

@polRk polRk commented Oct 11, 2024

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. If you can update it to only make the change to not set the header if the user already provided one, we can merge it.

I understand the idea, but we do not want to make a breaking change to user agent strings. Please avoid unnecessary changes to whitespace (npx turbo run format runs prettier).

@polRk
Copy link
Author

polRk commented Oct 14, 2024

Thanks for the PR. If you can update it to only make the change to not set the header if the user already provided one, we can merge it.

So, are you saying that the logic for generating the grpc-js-* string should be removed, and we should just pass the header instead?

@polRk polRk force-pushed the patch-1 branch 3 times, most recently from 376ead2 to fe51366 Compare October 14, 2024 13:59
@polRk polRk requested a review from timostamm October 14, 2024 13:59
@timostamm
Copy link
Member

So, are you saying that the logic for generating the grpc-js-* string should be removed

Yes.

and we should just pass the header instead?

Currently, we're always setting the User-Agent header, regardless of whether the user has already provided one. For example:

await client.say(
  { sentence: "hello" },
  {
    headers: {
      "User-Agent": "foo",
    }
  }
);

The User-Agent: foo is overwritten.

Following #1271 (comment), we should change the behavior, and not overwrite the user-provided value, but continue to set our value if the user didn't provide one.

The important part is that we do it consistently, not just in the gRPC transport, but also for gRPC-Web and the Connect transport.

Adding a transport option for a default user-agent is a nice idea, but it raises the question whether we shouldn't have an option for default headers instead. For now, the best action is to not add this option, and consider this change separately.

@polRk
Copy link
Author

polRk commented Oct 15, 2024

Currently, we're always setting the User-Agent header, regardless of whether the user has already provided one. For example:

The problem is:

  1. Passing headers would require changing the code throughout the entire codebase. In my previous project, I had thousands of gRPC requests, and modifying all of them would be difficult.

  2. Using interceptors introduces unnecessary overhead, especially when using AsyncLocalStorage.

Adding a transport option for a default user-agent is a nice idea, but it raises the question whether we shouldn't have an option for default headers instead. For now, the best action is to not add this option, and consider this change separately.

Yes, but, for example, I can pass headers directly:

server := grpc.Dial(grpc_server, WithUserAgent("user-agent",)) 

@timostamm
Copy link
Member

To fix #1271, we can allow to pass a custom user agent via CallOptions.headers, which is a reasonable incremental change.

We can discuss other features separately.

@polRk
Copy link
Author

polRk commented Oct 18, 2024

@timostamm

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Looks great.

Can you apply the same change to the Connect and gRPC-Web transports for consistency? It would be very confusing if this only works with gRPC. The transports from @connectrpc/connect-web don't set a user-agent, no need to update them.

@@ -34,7 +34,7 @@ describe("requestHeader", () => {
"user-agent",
]);
expect(headers.get("Content-Type")).toBe("application/grpc+proto");
expect(headers.get("User-Agent")).toMatch(/^connect-es\/\d+\.\d+\.\d+/);
expect(headers.get("User-Agent")).toMatch(/^connect-es\/.*/);
Copy link
Member

Choose a reason for hiding this comment

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

Let's not modify this test. It makes sure the default user agent doesn't regress.

@@ -48,6 +48,17 @@ describe("requestHeader", () => {
expect(headers.get("Grpc-Timeout")).toBe("10m");
});

it("should create request headers with userAgent", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the test!

// See https://github.com/grpc/grpc/blob/c462bb8d485fc1434ecfae438823ca8d14cf3154/doc/PROTOCOL-HTTP2.md#user-agents
result.set(headerUserAgent, "CONNECT_ES_USER_AGENT");

if (!result.has(headerUserAgent)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice and simple 👍

@polRk
Copy link
Author

polRk commented Oct 25, 2024

@timostamm Greetings! Will you be able to watch it?

polRk and others added 2 commits October 26, 2024 19:24
Signed-off-by: Vladislav Polyakov <[email protected]>
Signed-off-by: Vladislav Polyakov <[email protected]>
@timostamm timostamm changed the title fix: remove user-agent override Allow user-provided User-Agent request header Oct 28, 2024
Signed-off-by: Timo Stamm <[email protected]>
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

I was out of office last week. Let's get this merged.

Pushed 4862f94 to update the bundle size benchmarks.

@timostamm timostamm merged commit 92dcc1a into connectrpc:v2 Oct 28, 2024
41 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants