Skip to content
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

generic proxy: tracing support for the generic proxy based on the generic tracing #24790

Merged
merged 13 commits into from
Feb 14, 2023

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Jan 6, 2023

Commit Message: generic proxy: tracing support for the generic proxy based on the generic tracing
Additional Description:

Add tracing support to the generic proxy.

Risk Level: Low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: wait.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode wbpcode requested a review from htuch as a code owner January 6, 2023 12:27
@wbpcode
Copy link
Member Author

wbpcode commented Jan 6, 2023

Before formal working on this, there is a problem need to be clarified first.

Should I create a new Tracing proto message in the api/config/tracing/v3 or somewhere for protocol-independent usage?

Tracing should be protocol independent but current Tracing proto message in the api/extensions/filters/network/http_connnection_manager/v3 is HTTP specific.

cc @envoyproxy/api-shepherds @envoyproxy/api-watchers

And there is a related issue #22695. cc @mattklein123 do you have some new idea to this problem?

@htuch
Copy link
Member

htuch commented Jan 6, 2023

This is probably a place that we would refactor if we could I think. In the absence of that option, we could either deprecate the one in HCM and move to a more neutral location as you suggest (lots of impacted users, somewhat cleaner) or have generic proxy take an HCM proto dependency (fragile, logically nonsensical but probably the least impactul). The alternative, of maintaining two parallel tracing top-level config protos with the same fields, is not great, since it means perma-skew in features and is a PITA to maintain. Any other thoughts here?

@htuch htuch self-assigned this Jan 6, 2023
@mattklein123
Copy link
Member

or have generic proxy take an HCM proto dependency (fragile, logically nonsensical but probably the least impactul)

I would probably vote for this just to avoid churn. We have made similar compromises in the past like this.

@wbpcode
Copy link
Member Author

wbpcode commented Jan 8, 2023

or have generic proxy take an HCM proto dependency (fragile, logically nonsensical but probably the least impactul)

I would probably vote for this just to avoid churn. We have made similar compromises in the past like this.

It's acceptable for me.

But it would be better to record this to somewhere to tell future developers it's a compormise rather than a design or error. 🤔

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@KBaichoo
Copy link
Contributor

/wait

for formatting

@htuch I think this is waiting on your input

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

@repokitteh-read-only
Copy link

no relevant owners for "api"

🐱

Caused by: a #24790 (review) was submitted by @htuch.

see: more, trace.

@htuch
Copy link
Member

htuch commented Jan 13, 2023

@wbpcode there are a lot of TODOs and no tests yet, are you planning on adding more to this PR? If so, please LMK when ready for contrib review/merge, thanks.

@wbpcode
Copy link
Member Author

wbpcode commented Jan 13, 2023

@wbpcode there are a lot of TODOs and no tests yet, are you planning on adding more to this PR? If so, please LMK when ready for contrib review/merge, thanks.

Some pre-work need to be completed first in other PRs.

I will mark this as waiting before I complete everything. Thanks.

/wait

wbpcode added 4 commits February 3, 2023 09:45
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
wbpcode added 4 commits February 6, 2023 13:47
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Member Author

wbpcode commented Feb 8, 2023

This is ready for a review. And some core code modifications are split out to #25420.

cc @htuch cc @soulxu

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

@repokitteh-read-only
Copy link

no relevant owners for "api"

🐱

Caused by: a #24790 (review) was submitted by @htuch.

see: more, trace.

@htuch
Copy link
Member

htuch commented Feb 9, 2023

@soulxu can you take a look as CODEOWNER? Thanks.

@wbpcode
Copy link
Member Author

wbpcode commented Feb 9, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #24790 (comment) was created by @wbpcode.

see: more, trace.

@soulxu
Copy link
Member

soulxu commented Feb 13, 2023

no more question, LGTM, thanks

soulxu
soulxu previously approved these changes Feb 13, 2023
@wbpcode
Copy link
Member Author

wbpcode commented Feb 14, 2023

I think I forgot the changelog. I will add it.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Member Author

wbpcode commented Feb 14, 2023

Hi, @soulxu, could you give a new LGTM to this PR? Thanks. 🙇

@wbpcode
Copy link
Member Author

wbpcode commented Feb 14, 2023

And friendly ping @htuch

@wbpcode wbpcode merged commit 199a939 into envoyproxy:main Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants