tracing: update opencensus and googleapis, use SetName for operation name by kyessenov · Pull Request #7622 · envoyproxy/envoy

kyessenov · 2019-07-17T22:38:00Z

Description:
Un-pin opencensus and googleapis to use master versions
Use SetName span method to set route operation names (aligning with other tracers).

Risk Level: low
Testing: Unit tests
Docs Changes: None
Release Notes: None

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-07-17T22:38:36Z

cc @htuch @g-easy FYI

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-07-18T01:23:39Z

How do we update envoy-filter-example repo in lock-step? I need to update the WORKSPACE, since I can't load googleapis rules in the existing bazel/repositories.bzl (since it loads the googleapis).

kyessenov · 2019-07-18T01:35:13Z

Prepared envoyproxy/envoy-filter-example#94. I'm not sure how this can be done step-by-step since the two builds are coupled.

g-easy · 2019-07-18T02:12:40Z

bazel/repositories.bzl

    http_archive(
        name = "io_opencensus_cpp",
-        patch_args = ["-p0"],
-        patches = ["@envoy//bazel/foreign_cc:io_opencensus_cpp.patch"],


Nice. Thank you.

lizan

Thanks this is great, just one comment.

lizan · 2019-07-18T03:58:12Z

WORKSPACE


 go_register_toolchains(go_version = GO_VERSION)
+
+load("@com_google_googleapis//:repository_rules.bzl", "switched_rules_by_language")


You should be able to roll this into api_dependencies in bazel/repositories.bzl (not api/bazel/repositories.bzl).

api_dependencies has http_archive for com_google_googleapis, so that won't work. We can't load from a repo before it's http_archive.

and the naming is confusing. bazel/api_repositories.bzl does not actually load API repositories, so I get why one might get confused (I did).

Hey Lizan, do you still think it's possible to add a load from googleapis without WORKSPACE change? I don't see a way, since googleapis is loaded as one of the last calls in there.

Hmm I see what you're saying, I think we want to somehow roll this into a bzl file, @htuch do you have any idea? Just roll additional dependencies (those WORKSPACE rules after envoy_dependencies) in an additional bzl file should work but still somewhat ugly.

Gentle ping?

Yeah I think that would work, do you mind take a stab?

I don't understand the envoy_api hack well, I'd hope @htuch can help there. Either way, this is going to cause churn in importing repositories, even if we change from 2 calls to 3 calls.

Yes but less churning in in the future :)

mandarjog · 2019-07-18T13:37:18Z

source/extensions/tracers/opencensus/opencensus_tracer_impl.cc

-void Span::setOperation(absl::string_view operation) {
-  span_.AddAnnotation("setOperation", {{"operation", operation}});
-}
+void Span::setOperation(absl::string_view operation) { span_.SetName(operation); }


Nice!
Took a lot of Bazel magic to get this change through !

lizan

fix ci/WORKSPACE.filter.example to pass CI.

lizan · 2019-07-18T23:33:26Z

WORKSPACE


 go_register_toolchains(go_version = GO_VERSION)
+
+load("@com_google_googleapis//:repository_rules.bzl", "switched_rules_by_language")


Hmm I see what you're saying, I think we want to somehow roll this into a bzl file, @htuch do you have any idea? Just roll additional dependencies (those WORKSPACE rules after envoy_dependencies) in an additional bzl file should work but still somewhat ugly.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-07-18T23:45:41Z

Thanks for the pointer. I've updated the example filter workspace.

kyessenov · 2019-07-19T02:30:22Z

/retest

repokitteh-read-only · 2019-07-19T02:30:26Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7622 (comment) was created by @kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-07-23T20:49:03Z

Is there any concern with this PR? We'd really benefit from this being in the upstream, since it simplifies bazel hacking in downstream projects.

lizan

I was thinking rolling everything else (foreign_cc / go_rules / rbe / googleapi_import) into a new bzl so there will be less churn. defer to @htuch

htuch

Thanks for the upgrade, I have a few questions.

api/bazel/api_build_system.bzl

htuch · 2019-07-24T07:58:03Z

bazel/api_binding.bzl

+    if "envoy_api" not in native.existing_rules().keys():
+        _default_envoy_api(name = "envoy_api")
+
+    native.bind(


Do we need to still do this bind? Can we replace the bind uses inside the API tree with the canonical @com_google_apis//... labels? Same below.

This would be fine as long as none of downstream envoy repositories refer to the bind. I don't have confidence that is the case, so I left it as-is for backwards compatibility reasons.

I'm happy to say "go ahead" on this, we don't provide stability guarantees around WORKSPACE and you're moving towards the long-term stable canonical naming convention in Bazel.

This is where recursive workspaces would be great.

Unfortunately, grpc_json_transcoder depends on this binding, so I can't remove it without making changes to that repository. Can this be done later since we'll have to make changes to other repositories first?

Ack, can you file an issue for cleanup here and tag the site with TODO?

Filed #7719 and referenced it here.

kyessenov · 2019-07-24T08:12:48Z

@lizan I still don't get what you are trying to suggest. Bazel requires load to be first in every .bzl file. That means you have to have multiple .bzl files as you load your dependent repositories with their .bzl files. How would folding everything into one file avoid this issue?

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-07-24T20:40:17Z

/retest

repokitteh-read-only · 2019-07-24T20:40:23Z

🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #7622 (comment) was created by @kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan · 2019-07-24T22:32:29Z

My intention was rolling everything after

envoy/WORKSPACE

Line 11 in e0cae9c

    
           load("@rules_foreign_cc//:workspace_definitions.bzl", "rules_foreign_cc_dependencies")

to a new .bzl file where you can load bzl from repositories which are already loaded by envoy_dependencies, thus we have smaller WORKSPACE` and future changes can just made there.

htuch · 2019-07-25T14:27:11Z

@lizan @kyessenov I would be in favor of shrinking down to the smallest possible WORKSPACE. The question is how few lines can we get away with. @kyessenov is correct that load must be at the top of any .bzl. I wonder if we could transform:

WORKSPACE:

load("a.bzl", a)
a()
load("b.bzl", b)
b()

to

WORKSPACE:

load("c.bzl", c)
c()

c.bzl:

load("a.bzl", "a")
load("b.bzl", "b")
a()
b()

I.e. can we arbitrarily reorder the loads?

kyessenov · 2019-07-25T16:40:30Z

load("//bazel:api_binding.bzl", "envoy_api_binding")
load("//bazel:api_repositories.bzl", "envoy_api_dependencies")

envoy_api_binding()
envoy_api_dependencies()

gives an error:

ERROR: Failed to load Starlark extension '@envoy_api//bazel:repositories.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @envoy_api

I guess the answer is no, the order of loads is important.

htuch · 2019-07-25T17:07:32Z

Crap, I see the issue. We have load statements that rely on .bzl from earlier imported dependencies.

@lizan I don't think we can do this in a .bzl, given this dependency. I think we should go with things as they are.

@dslomov this seems to be a pretty big limitation without recursive workspaces. I recall that recurisve workspaces had issues, do you have the latest best practice here?

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch

LGTM given constraints, @lizan are you good to merge?

lizan · 2019-07-25T21:19:01Z

@htuch @kyessenov this is what I meant (and it works): e45ad99

so we only have 3 load + function calls, and the WORKSPACE shouldn't grow more...

kyessenov · 2019-07-25T21:22:03Z

@lizan What happens when we need to import from something loaded in envoy_dependency_imports? :) I think this is pretty arbitrary to have 4 top-level calls, and it happens to work because our load-import chain is only 3-long. But we need a systematic solution here...

lizan · 2019-07-25T21:41:16Z

Yeah it is theoretically possible, but I think this is best we can have before recursive workspace. I don't think we should or want to have an external dependency with that complexity (i.e. 3-level dependency from) by then.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-07-25T22:11:26Z

@lizan updated to make workspaces smaller.

lizan · 2019-07-25T22:18:22Z

Do we still need the api_binding / api_dependencies split? I'm fine with the renaming original api_dependencies -> api_binding, but the split seems unnecessary.

kyessenov · 2019-07-25T22:20:49Z

I'd prefer to keep api_dependencies distinct from dependencies since I want to be able to build just APIs. The binding trick should also be kept separate since I may not want to do the local bind that envoy does. Since any two of the three things cannot be placed into one bzl file, I think it's fair to keep them separate.

lizan

I'd prefer to keep api_dependencies distinct from dependencies since I want to be able to build just APIs. The binding trick should also be kept separate since I may not want to do the local bind that envoy does. Since any two of the three things cannot be placed into one bzl file, I think it's fair to keep them separate.

OK that's fair.

lizan · 2019-07-25T22:35:51Z

bazel/dependency_imports.bzl

+load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
+load("@envoy//bazel/toolchains:rbe_toolchains_config.bzl", "rbe_toolchains_config")
+
+def envoy_dependency_imports(go_version = "1.12.5"):


just move GO_VERSION from bazel/repositories.bzl to here? It is not used anywhere else.

Signed-off-by: Kuat Yessenov <kuat@google.com>

Mythra · 2019-07-26T14:44:34Z

I believe these changes broke envoy-filter-example (as well as our internal projects that use a similar workspace to envoy-filter-example).

As of this PR being out envoy-filter-example & others now fail with:

ERROR: Failed to load Starlark extension '@envoy_api//bazel:repositories.bzl'.
Cycle in the workspace file detected. This indicates that a repository is used prior to being defined.
The following chain of repository dependencies lead to the missing definition.
 - @envoy_api
This could either mean you have to add the '@envoy_api' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.
ERROR: cycles detected during target parsing
INFO: Elapsed time: 0.040s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)

Even with applying the PR: envoyproxy/envoy-filter-example#94 locally, it still fails to build.

kyessenov · 2019-07-26T17:11:10Z

Please take a look at the diff in ci/WORKSPACE.filter.example for the change required in the workspace. You can read the discussion above about why it is hard to keep it backwards compatible.

Mythra · 2019-07-26T17:13:01Z

Yeah, I missed that originally, but it was mentioned in slack. There’s now a PR to the filter example that fixed it, and we’ve fixed internally.

Sorry for missing that!

kyessenov added 3 commits July 16, 2019 22:43

WIP

4b80cad

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix envoy_api

a7bc4c3

Signed-off-by: Kuat Yessenov <kuat@google.com>

use setName from opencensus

d0eb12f

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

e0cae9c

Signed-off-by: Kuat Yessenov <kuat@google.com>

zuercher assigned htuch Jul 18, 2019

g-easy reviewed Jul 18, 2019

View reviewed changes

lizan suggested changes Jul 18, 2019

View reviewed changes

mandarjog reviewed Jul 18, 2019

View reviewed changes

lizan reviewed Jul 18, 2019

View reviewed changes

kyessenov added 2 commits July 18, 2019 16:36

fix filter example

eb939bd

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

d50582b

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 4 commits July 19, 2019 13:38

merge fix

a49cde6

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into update_opencensus

0b76f1d

review feedback

cf6fa68

Signed-off-by: Kuat Yessenov <kuat@google.com>

typo

3cfba6a

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan reviewed Jul 23, 2019

View reviewed changes

htuch suggested changes Jul 24, 2019

View reviewed changes

kyessenov added 2 commits July 24, 2019 13:05

Merge remote-tracking branch 'upstream/master' into update_opencensus

bf085e2

review feedback

404b7da

Signed-off-by: Kuat Yessenov <kuat@google.com>

revert opencensus cpp change

e5a4c31

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov mentioned this pull request Jul 25, 2019

filter: add conditions to access control filter #7716

Merged

htuch added the waiting:any label Jul 25, 2019

repokitteh-read-only bot removed the waiting:any label Jul 25, 2019

add TODO

585aab5

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch previously approved these changes Jul 25, 2019

View reviewed changes

kyessenov added 2 commits July 25, 2019 15:07

Merge remote-tracking branch 'upstream/master' into update_opencensus

7d25a9d

update workspaces

6524c56

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed htuch’s stale review via 6524c56 July 25, 2019 22:11

lizan reviewed Jul 25, 2019

View reviewed changes

move GO_VERSION

7241003

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan approved these changes Jul 25, 2019

View reviewed changes

lizan merged commit ef054f0 into envoyproxy:master Jul 26, 2019


		go_register_toolchains(go_version = GO_VERSION)

		load("@com_google_googleapis//:repository_rules.bzl", "switched_rules_by_language")

Conversation

kyessenov commented Jul 17, 2019

Uh oh!

kyessenov commented Jul 17, 2019

Uh oh!

kyessenov commented Jul 18, 2019

Uh oh!

kyessenov commented Jul 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Jul 18, 2019

Uh oh!

kyessenov commented Jul 19, 2019

Uh oh!

repokitteh-read-only bot commented Jul 19, 2019

Uh oh!

kyessenov commented Jul 23, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Jul 24, 2019

Uh oh!

kyessenov commented Jul 24, 2019

Uh oh!

repokitteh-read-only bot commented Jul 24, 2019

Uh oh!

lizan commented Jul 24, 2019

Uh oh!

htuch commented Jul 25, 2019

Uh oh!

kyessenov commented Jul 25, 2019

kyessenov commented Jul 26, 2019 •

edited

Loading