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

interop/xds: Interop client and server changes for CSM Observability #7280

Merged
merged 10 commits into from
May 31, 2024

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented May 29, 2024

This PR adds the CSM Observability flags for interop testing. It makes the xDS interop client and server their own go.mod to avoid upgrading the minimum required go version of the top level grpc/ go.mod to require go 1.21, as the client and server take a transitive OpenTelemetry dependency through the csm package, which requires go 1.21.

RELEASE NOTES: N/A

@zasweq zasweq added this to the 1.65 Release milestone May 29, 2024
@zasweq zasweq requested a review from dfawley May 29, 2024 23:44
@zasweq zasweq force-pushed the interop-changes-csm-o11y branch from 93a2f4b to 59e5a4c Compare May 30, 2024 00:33
interop/xds/client/Dockerfile Outdated Show resolved Hide resolved
interop/xds/client/Dockerfile Outdated Show resolved Hide resolved
interop/xds/client/client.go Outdated Show resolved Hide resolved
interop/xds/client/client.go Outdated Show resolved Hide resolved
interop/xds/client/go.mod Outdated Show resolved Hide resolved
interop/xds/server/Dockerfile Outdated Show resolved Hide resolved
interop/xds/server/go.mod Outdated Show resolved Hide resolved
@dfawley dfawley assigned zasweq and unassigned dfawley May 30, 2024
interop/xds/go.mod Outdated Show resolved Hide resolved
@zasweq zasweq assigned dfawley and unassigned zasweq May 30, 2024
@zasweq
Copy link
Contributor Author

zasweq commented May 30, 2024

Thanks for the pass sorry for the hassle; interop still works on latest commit

Copy link
Member

Choose a reason for hiding this comment

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

Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, deleted.

provider := metric.NewMeterProvider(
metric.WithReader(exporter),
)
go http.ListenAndServe(":9464", promhttp.Handler())
Copy link
Member

Choose a reason for hiding this comment

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

Please add port flag and use it here.

Copy link
Member

Choose a reason for hiding this comment

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

*or environment variable, whatever is decided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to do env var "OTEL_EXPORTER_PROMETHEUS_PORT" with default of 9464. Interop test won't set and will default.

provider := metric.NewMeterProvider(
metric.WithReader(exporter),
)
go http.ListenAndServe(":9464", promhttp.Handler())
Copy link
Member

Choose a reason for hiding this comment

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

Should we do something like go func() { if err := ListenAndServe; err != nil { logger.Fatalf } }(), so if the port is taken we get a noisy error instead of just things not working right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for both client and server.

interop/xds/client/client.go Show resolved Hide resolved
@zasweq zasweq assigned dfawley and unassigned dfawley May 31, 2024
@zasweq
Copy link
Contributor Author

zasweq commented May 31, 2024

Verified recent commits with another interop pass.

@@ -24,13 +24,13 @@ COPY . .

# Build a static binary without cgo so that we can copy just the binary in the
# final image, and can get rid of Go compiler and gRPC-Go dependencies.
RUN go build -tags osusergo,netgo interop/xds/client/client.go
RUN cd interop/xds/client && go build -tags osusergo,netgo client.go
Copy link
Member

Choose a reason for hiding this comment

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

How about .?

I think if you specify the file here and we add another file, it will break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -24,13 +24,13 @@ COPY . .

# Build a static binary without cgo so that we can copy just the binary in the
# final image, and can get rid of the Go compiler and gRPC-Go dependencies.
RUN go build -tags osusergo,netgo interop/xds/server/server.go
RUN cd interop/xds/server && go build -tags osusergo,netgo server.go
Copy link
Member

Choose a reason for hiding this comment

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

As above, build . is probably better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dfawley dfawley assigned zasweq and unassigned dfawley May 31, 2024
@zasweq zasweq force-pushed the interop-changes-csm-o11y branch from e76b1b6 to 4484a86 Compare May 31, 2024 21:41
@zasweq
Copy link
Contributor Author

zasweq commented May 31, 2024

Continues to pass; merging.

@zasweq zasweq merged commit 5d7bd7a into grpc:master May 31, 2024
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants