gcp/observability: correctly test this module in presubmit tests

[lidizheng]

In my manual testing of the plugin, I found I made some mistakes in #5196. I didn't check how the tests are picked by the GitHub presubmit checks, so no gcp/observability tests were run.

After the path changed, I forgot to update some imports, however, my local unit tests continued to pass because I didn't clean the object files.

This PR: 1. added gcp/observability tests to presubmit; 2. fix the import typos; 3. fix the WriteLogEntries path; 4. fix the grpc release version requirement.

RELEASE NOTES: N/A

[lidizheng]

@dfawley PTAL.

The failed check looks like a flake.

[menghanl]

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

[lidizheng]

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

[dfawley]

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?

[lidizheng]

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...

[lidizheng]

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.

[menghanl]

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).

[dfawley]

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?

[lidizheng]

@menghanl @dfawley PTALA. Let me know what else can I do.
(This PR would need to be backported to the v1.46.x branch, I will create the backport PR once this is merged.)

[menghanl]

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

[lidizheng]

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)

[menghanl]

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