chore(deps): Bump gRPC to v1.80.0#3270
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3270 +/- ##
==========================================
- Coverage 27.38% 27.31% -0.07%
==========================================
Files 95 95
Lines 5427 5425 -2
Branches 2548 2548
==========================================
- Hits 1486 1482 -4
- Misses 3214 3216 +2
Partials 727 727
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d3e3490 to
6ab05ea
Compare
Update gRPC from v1.67.0 to v1.80.0, along with its required dependency bumps: protobuf v28.3 to v31.1 and abseil-cpp 20240722.0 to 20250512.1. Adapt collector code to protobuf v31 breaking changes: - Arena::CreateMessage<T>() removed, replaced with Arena::Create<T>() - google::protobuf::uint64 removed, replaced with uint64_t - FieldDescriptor::name() now returns absl::string_view instead of const std::string&
6ab05ea to
c5db7d5
Compare
|
/retest collector-on-push |
| if (value == nullptr) { | ||
| ParserError err; | ||
| err << file_ << ": Invalid enum value '" << enum_name << "' for field " << (read_camelcase_ ? SnakeCaseToCamel(field->name()) : field->name()); | ||
| err << file_ << ": Invalid enum value '" << enum_name << "' for field " << (read_camelcase_ ? SnakeCaseToCamel(std::string(field->name())) : std::string(field->name())); |
There was a problem hiding this comment.
This is not a huge deal because it is in the error side of a configuration reload which should not happen very often, but do we need to build std::string because the field->name() method is returning an abseil string_view? Can we create std::string_view instead so we don't have to copy the entire string over?
There was a problem hiding this comment.
Yes, field->name() used to return std::string, and after the protobuf bump it's a absl:string_view (which I think it's just a typedef to std::string_view).
But I'd also have to modify SnakeCaseToCamel to take std::string_view instead of const std::string&. Should I do that?
There was a problem hiding this comment.
I think it could be worth it, right now the case where read_camelcase_ is true creating a new std::string before calling SnakeCaseToCamel and this in turn will create a separate string with the case changed (the compiler might be smart enough to not do these multiple allocations, but I would rather not rely on it).
We still need to create a std::string for the false case because SnakeCaseToCamel needs to return that type and the compiler will get upset if we have different types as the result of the branches in a ternary operator.
I'll leave it up to you, I think I'm just putting too much effort on a piece of code that will most likely not be hit and is not in a hot path.
|
|
||
| std::string camel; | ||
| const std::string* name = &field->name(); | ||
| std::string name(field->name()); |
There was a problem hiding this comment.
Similar to my previous comment.
There was a problem hiding this comment.
Also here, SnakeCaseToCamel, ConcatPath and ParseScalar should then be modified to use std::string_view instead of const std::string&.
And it looks like we'd still need a copy on the read_camelcase_ path:
std::string name(field->name());
if (read_camelcase_) {
name = SnakeCaseToCamel(name);
}
There was a problem hiding this comment.
Same as my answer to the previous comment, I'll leave it up to you, it might not be worth the effort since this is not code that is on any hot path.
Description
Update gRPC from v1.67.0 to v1.80.0, along with its required dependency bumps:
protobufv28.3 to v31.1abseil-cpp20240722.0 to 20250512.1.The
abseil-cppbump triggers GCC bug #71962 (tracked in abseil#1634):-fsanitize=undefinedbreaksconstexprevaluation of function pointer comparisons in abseil'shash_policy_traits.h, causing a build failure inConfigLoaderTest.cppwhen building with address sanitizer enabled. Worked around by disabling UBSan for that file only.Adapt collector code to protobuf v31 breaking changes:
Arena::CreateMessage<T>()removed, replaced withArena::Create<T>()google::protobuf::uint64removed, replaced withuint64_tFieldDescriptor::name()now returnsabsl::string_viewinstead ofconst std::string&This bump will make it easier to upgrade to v1.81.0 when it'll be available (which will include required PQC fixes). It could also be merged before #3269, since in that PR I used some APIs that are now deprecated.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
CI is sufficient.