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

gcp/observability: correctly test this module in presubmit tests #5300

Merged
merged 4 commits into from Apr 12, 2022
Merged

gcp/observability: correctly test this module in presubmit tests #5300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fc5d9b
gcp/observability: correctly test this module in presubmit tests
lidizheng Apr 6, 2022
2e9a37e
Try out the auto-go-mod discovery script
lidizheng Apr 8, 2022
6941c3c
Add a remove todo
lidizheng Apr 8, 2022
5185247
Update the testing.yml to correct the go.mod file exclusion
lidizheng Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions .github/workflows/testing.yml
View file
Open in desktop
Expand Up @@ -106,9 +106,12 @@ jobs:
run: |
go version
go test ${{ matrix.testflags }} -cpu 1,4 -timeout 7m google.golang.org/grpc/...
cd ${GITHUB_WORKSPACE}/security/advancedtls && go test ${{ matrix.testflags }} -timeout 2m google.golang.org/grpc/security/advancedtls/...
cd ${GITHUB_WORKSPACE}/security/authorization && go test ${{ matrix.testflags }} -timeout 2m google.golang.org/grpc/security/authorization/...

cd "${GITHUB_WORKSPACE}"
for MOD_FILE in $(find . -name 'go.mod' | grep -Ev '^\./go\.mod'); do
Copy link
Contributor

Choose a reason for hiding this comment

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

This will run the top level tests again.
Limit the find?
find . -mindepth 2 -name 'go.mod'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the suggested snippet, the second part is grep -Ev '^\./go\.mod', it excludes the top level tests. Can you double check if current test set works as expected? https://github.com/grpc/grpc-go/runs/5994752478?check_suite_focus=true

(It looks reasonable to me, no duplication found)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. I somehow thought that was to remove the go.mod and keep only the directory name. This should be good then!

pushd "$(dirname ${MOD_FILE})"
go test ${{ matrix.testflags }} -timeout 2m ./...
popd
done

# Non-core gRPC tests (examples, interop, etc)
- name: Run extras tests
Expand Down
2 changes: 1 addition & 1 deletion gcp/observability/config.go
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ import (

gcplogging "cloud.google.com/go/logging"
"golang.org/x/oauth2/google"
configpb "google.golang.org/grpc/observability/internal/config"
configpb "google.golang.org/grpc/gcp/observability/internal/config"
"google.golang.org/protobuf/encoding/protojson"
)

Expand Down
2 changes: 1 addition & 1 deletion gcp/observability/exporting.go
View file
Open in desktop
Expand Up @@ -25,7 +25,7 @@ import (
"os"

gcplogging "cloud.google.com/go/logging"
grpclogrecordpb "google.golang.org/grpc/observability/internal/logging"
grpclogrecordpb "google.golang.org/grpc/gcp/observability/internal/logging"
"google.golang.org/protobuf/encoding/protojson"
)

Expand Down
6 changes: 4 additions & 2 deletions gcp/observability/go.mod
View file
Open in desktop
@@ -1,4 +1,4 @@
module google.golang.org/grpc/observability
module google.golang.org/grpc/gcp/observability

go 1.14

Expand All @@ -7,8 +7,10 @@ require (
github.com/golang/protobuf v1.5.2
github.com/google/uuid v1.3.0
golang.org/x/oauth2 v0.0.0-20211104180415-d3ed0bb246c8
google.golang.org/grpc v1.43.0
google.golang.org/grpc v1.46.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this fail to compile? Since 1.43.0 doesn't have the exported binarylog structs.
Or this was hidden by the replace below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep... this unfortunately is hidden by the replace.

Copy link
Member

Choose a reason for hiding this comment

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

So if we make changes in gcp/observability in the future that rely on changes in the main module, we won't ever catch that? Should we delete the replace directive entirely to catch that? Also does that mean we can't make backward-incompatible changes to our own internal packages now, because a user might be using this version of observability with grpc v1.51.0 one day? Or do we need to make observability a dependency of the main module and keep them perfectly in sync when we do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a TODO to remove the replace.

does that mean we can't make backward-incompatible changes to our own internal packages now.

It would be great to decouple them (to allow internal packages to continue be internal). But I'm not sure what's the best practice here. We can strive to use let it use public API in future (by exposing experimental APIs).

Or do we need to make observability a dependency of the main module and keep them perfectly in sync when we do that?

Does this solution mean gRPC Observability will impact the dependency tree of all gRPC Go users? That doesn't sound good...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Giving this another thought. We could aim to only use public methods for future plugins like the observability one? In this case, we were using the internal methods of binary logging to customize the logger. We didn't public them, because we were uncertain if we will keep using this design (wrap binary logging) or if the plugin will stay for the long run. We could find a middle ground here to publicize some hooks if they help users to extend those modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Making the APIs public doesn't make this situation better, because changing them will still break our own observability module.

From another perspective, maybe this isn't that bad, since at least for now, the observability package is still experimental. And also, we don't make separate releases for the observability module (so that even if it's a separate module, it shared the version of the main module).

google.golang.org/protobuf v1.27.1
)

// TODO(lidiz) remove the following line when we have a release containing the
// necessary internal binary logging changes
replace google.golang.org/grpc => ../../
6 changes: 3 additions & 3 deletions gcp/observability/logging.go
View file
Open in desktop
Expand Up @@ -27,9 +27,9 @@ import (

"github.com/google/uuid"
binlogpb "google.golang.org/grpc/binarylog/grpc_binarylog_v1"
configpb "google.golang.org/grpc/gcp/observability/internal/config"
grpclogrecordpb "google.golang.org/grpc/gcp/observability/internal/logging"
iblog "google.golang.org/grpc/internal/binarylog"
configpb "google.golang.org/grpc/observability/internal/config"
grpclogrecordpb "google.golang.org/grpc/observability/internal/logging"
)

// translateMetadata translates the metadata from Binary Logging format to
Expand Down Expand Up @@ -203,7 +203,7 @@ func (l *binaryLogger) GetMethodLogger(methodName string) iblog.MethodLogger {
// we batch up the uploads in the exporting RPC, the message content of that
// RPC will be logged. Without this exclusion, we may end up with an ever
// expanding message field in log entries, and crash the process with OOM.
if methodName == "google.logging.v2.LoggingServiceV2/WriteLogEntries" {
if methodName == "/google.logging.v2.LoggingServiceV2/WriteLogEntries" {
return ol
}

Expand Down
4 changes: 2 additions & 2 deletions gcp/observability/observability_test.go
View file
Open in desktop
Expand Up @@ -30,14 +30,14 @@ import (

"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
configpb "google.golang.org/grpc/gcp/observability/internal/config"
grpclogrecordpb "google.golang.org/grpc/gcp/observability/internal/logging"
iblog "google.golang.org/grpc/internal/binarylog"
"google.golang.org/grpc/internal/grpctest"
"google.golang.org/grpc/internal/leakcheck"
testgrpc "google.golang.org/grpc/interop/grpc_testing"
testpb "google.golang.org/grpc/interop/grpc_testing"
"google.golang.org/grpc/metadata"
configpb "google.golang.org/grpc/observability/internal/config"
grpclogrecordpb "google.golang.org/grpc/observability/internal/logging"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/proto"
Expand Down