Tracing: allow proper tracing for ratelimit calls from Envoy#486
Merged
RomanDzhabarov merged 9 commits intomasterfrom Feb 16, 2017
Merged
Tracing: allow proper tracing for ratelimit calls from Envoy#486RomanDzhabarov merged 9 commits intomasterfrom
RomanDzhabarov merged 9 commits intomasterfrom
Conversation
include/envoy/ratelimit/ratelimit.h
Outdated
| virtual void limit(RequestCallbacks& callbacks, const std::string& domain, | ||
| const std::vector<Descriptor>& descriptors, | ||
| const std::string& request_id) PURE; | ||
| const std::vector<Descriptor>& descriptors, const std::string& request_id, |
Member
There was a problem hiding this comment.
Instead of adding another param, I think at this point it would be better to make something like:
struct TraceContext {
std::string request_id_;
std::string span_context_;
};
And pass a const TraceContext&
We may add other things in the future and this will end up being a lot cleaner.
Member
Author
There was a problem hiding this comment.
Deal, I'll name that TransportContext and will place that in Tracing namespace
RomanDzhabarov
commented
Feb 15, 2017
include/envoy/ratelimit/ratelimit.h
Outdated
| * @param descriptors specifies a list of descriptors to query. | ||
| * @param request_id propagates the request_id of the current request to the ratelimit service. | ||
| * @param context provides transport context so that upstream can construct proper relationship | ||
| * between |
RomanDzhabarov
commented
Feb 15, 2017
| @@ -1,3 +1,4 @@ | |||
| #include <envoy/tracing/http_tracer.h> | |||
RomanDzhabarov
commented
Feb 15, 2017
|
|
||
| callbacks_->complete(status); | ||
| callbacks_ = nullptr; | ||
| request_id_.clear(); |
Member
Author
There was a problem hiding this comment.
this is not required as we set it on every limit request
include/envoy/tracing/http_tracer.h
Outdated
| * Transport tracing context. | ||
| * It's used to set proper parent/child span relationship on Envoy calls, e.g., ratelimit call. | ||
| */ | ||
| struct TransportContext { |
Member
There was a problem hiding this comment.
this should go in a header not called http_tracer.h
source/common/filter/ratelimit.cc
Outdated
| config_->stats().total_.inc(); | ||
| calling_limit_ = true; | ||
| client_->limit(*this, config_->domain(), config_->descriptors(), EMPTY_STRING); | ||
| client_->limit(*this, config_->domain(), config_->descriptors(), {EMPTY_STRING, EMPTY_STRING}); |
include/envoy/tracing/http_tracer.h
Outdated
| std::string request_id_; | ||
| std::string span_context_; | ||
|
|
||
| bool operator==(const Tracing::TransportContext& rhs) const { |
Member
There was a problem hiding this comment.
since this is only needed in test code, put the definition of operator== in a test header.
test/mocks/ratelimit/mocks.h
Outdated
| return lhs.entries_ == rhs.entries_; | ||
| } | ||
|
|
||
| inline bool operator()(const Tracing::TransportContext& lhs, |
Member
There was a problem hiding this comment.
This goes in a file in the tracing namespace
source/common/filter/ratelimit.cc
Outdated
| config_->stats().total_.inc(); | ||
| calling_limit_ = true; | ||
| client_->limit(*this, config_->domain(), config_->descriptors(), EMPTY_STRING); | ||
| client_->limit(*this, config_->domain(), config_->descriptors(), {"", ""}); |
Member
There was a problem hiding this comment.
{} should work, but either way, I should have commented on this before, define a static const empty context, and pass it here.
Member
Author
There was a problem hiding this comment.
yeah in this case for some reason that does not work, compiler complains about missing initializer for request_id and span_context_
mattklein123
approved these changes
Feb 16, 2017
lizan
pushed a commit
to lizan/envoy
that referenced
this pull request
Apr 24, 2020
fixes to filter state getters/setters
wolfguoliang
pushed a commit
to wolfguoliang/envoy
that referenced
this pull request
Jan 23, 2021
jpsim
pushed a commit
that referenced
this pull request
Nov 28, 2022
Pulls in the fix from #8467. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
pushed a commit
that referenced
this pull request
Nov 29, 2022
Pulls in the fix from #8467. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
mathetake
pushed a commit
that referenced
this pull request
Mar 3, 2026
**Commit Message** Content type should be able to be string so adding this for compatibility. It's already done for the other types - keeping it consistent. --------- Signed-off-by: Aaron Choo <achoo30@bloomberg.net> Co-authored-by: Dan Sun <dsun20@bloomberg.net>
mathetake
pushed a commit
that referenced
this pull request
Mar 3, 2026
…508) **Commit Message** AWS Claude 3.5 v1 has cases when assistant returns empty content. Sending that back to AWS will result in a validation error. Will skip adding empty content if string is empty. ``` openai.BadRequestError: Error code: 400 - {'type': 'error', 'error': {'type': 'ValidationException:http://internal.amazon.com/coral/com.amazon.bedrock/', 'code': '400', 'message': 'The text field in the ContentBlock object at messages.3.content.0 is blank. Add text to the text field, and try again.'}} ``` **Related Issues/PRs (if applicable)** #486 Signed-off-by: Aaron Choo <achoo30@bloomberg.net>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
@lyft/network-team
This is to propagate x-ot-span-context on calls to ratelimit, so ingress Envoy on ratelimit service can set proper parent for the span.