-
Notifications
You must be signed in to change notification settings - Fork 5.5k
tracing: update opencensus and googleapis, use SetName for operation name #7622
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
4b80cad
WIP
kyessenov a7bc4c3
fix envoy_api
kyessenov d0eb12f
use setName from opencensus
kyessenov e0cae9c
fix format
kyessenov eb939bd
fix filter example
kyessenov d50582b
merge fix
kyessenov a49cde6
merge fix
kyessenov 0b76f1d
Merge remote-tracking branch 'upstream/master' into update_opencensus
kyessenov cf6fa68
review feedback
kyessenov 3cfba6a
typo
kyessenov bf085e2
Merge remote-tracking branch 'upstream/master' into update_opencensus
kyessenov 404b7da
review feedback
kyessenov e5a4c31
revert opencensus cpp change
kyessenov 585aab5
add TODO
kyessenov 7d25a9d
Merge remote-tracking branch 'upstream/master' into update_opencensus
kyessenov 6524c56
update workspaces
kyessenov 7241003
move GO_VERSION
kyessenov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,6 +160,7 @@ def envoy_dependencies(skip_targets = []): | |
| _python_deps() | ||
| _cc_deps() | ||
| _go_deps(skip_targets) | ||
|
|
||
| api_dependencies() | ||
|
|
||
| def _boringssl(): | ||
|
|
@@ -515,8 +516,6 @@ def _io_opencensus_cpp(): | |
| location = REPOSITORY_LOCATIONS["io_opencensus_cpp"] | ||
| http_archive( | ||
| name = "io_opencensus_cpp", | ||
| patch_args = ["-p0"], | ||
| patches = ["@envoy//bazel/foreign_cc:io_opencensus_cpp.patch"], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. Thank you. |
||
| **location | ||
| ) | ||
| native.bind( | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to roll this into
api_dependenciesinbazel/repositories.bzl(notapi/bazel/repositories.bzl).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api_dependencieshashttp_archivefor com_google_googleapis, so that won't work. We can't load from a repo before it's http_archive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I see what you're saying, I think we want to somehow roll this into a
bzlfile, @htuch do you have any idea? Just roll additional dependencies (those WORKSPACE rules afterenvoy_dependencies) in an additionalbzlfile should work but still somewhat ugly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gentle ping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that would work, do you mind take a stab?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but less churning in in the future :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, PTAL