Skip to content

[feat] using exporter-toolkit/web for web config #2165

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

Open
wants to merge 1 commit into
base: reboot
Choose a base branch
from

Conversation

SamYuan1990
Copy link
Collaborator

Let's use this PR to discuss how we integrate with exporter-toolkit/web.
From exporter-toolkit/web SDK, we need *web.FlagConfig structure ready before we start the server.
but the question here is how we make it in our configuration and code structure.
we now have config in yaml file and those flag reg in same pattern, if we fully moved to exporter-toolkit/web as one example impl in this PR shows.
We still need all config in one place, so that it will not impact main.go, but somehow breaks config.go in yaml and flag reg process.
The benefits here IMO, is that we can leave web config to upstream repo for feature supports and it seems for an exporter we don't need to support listen on multi socket, so... just a default port and default TLS config file will be good enough.

@SamYuan1990 SamYuan1990 changed the title [chore] using exporter-toolkit/web for web config [WIP][chore] using exporter-toolkit/web for web config Jun 18, 2025
@SamYuan1990 SamYuan1990 marked this pull request as ready for review June 18, 2025 02:27
@SamYuan1990 SamYuan1990 requested review from sthaha and vimalk78 June 18, 2025 02:27
@SamYuan1990
Copy link
Collaborator Author

@sthaha and @vimalk78 may I have your feedback on this PR? I know there some test case need to be fix, but maybe we need to vote for using exporter-toolkit/web for web config or not and make a design for how integration it.

@sthaha
Copy link
Collaborator

sthaha commented Jun 18, 2025

@sthaha and @vimalk78 may I have your feedback on this PR? I know there some test case need to be fix, but maybe we need to vote for using exporter-toolkit/web for web config or not and make a design for how integration it.

I like the idea of embedding webconfig into the config. It is one less configuration to worry about. Let me play around it a bit and get back to you.


Just a nit: ... This PR will be a feat and not a chore https://gist.github.com/qoomon/5dfcdf8eec66a051ecd85625518cfd13#types

@SamYuan1990 SamYuan1990 force-pushed the tlsconfigToexporter-toolkit branch from d177438 to ff6cd04 Compare June 20, 2025 08:16
@github-actions github-actions bot added the chore Routine tasks or maintenance label Jun 20, 2025
@SamYuan1990 SamYuan1990 force-pushed the tlsconfigToexporter-toolkit branch from ff6cd04 to fa081d9 Compare June 20, 2025 08:25
@SamYuan1990 SamYuan1990 force-pushed the tlsconfigToexporter-toolkit branch from fa081d9 to 9c68f7f Compare June 20, 2025 08:50
Copy link

codecov bot commented Jun 20, 2025

Codecov Report

Attention: Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.05%. Comparing base (7ed9575) to head (9c68f7f).
Report is 10 commits behind head on reboot.

Files with missing lines Patch % Lines
internal/server/server.go 90.90% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           reboot    #2165      +/-   ##
==========================================
+ Coverage   91.51%   92.05%   +0.53%     
==========================================
  Files          36       36              
  Lines        3490     3612     +122     
==========================================
+ Hits         3194     3325     +131     
+ Misses        229      224       -5     
+ Partials       67       63       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SamYuan1990 SamYuan1990 changed the title [WIP][chore] using exporter-toolkit/web for web config [chore] using exporter-toolkit/web for web config Jun 20, 2025
@SamYuan1990
Copy link
Collaborator Author

@sthaha and @vimalk78 may I have your feedback on this PR? I know there some test case need to be fix, but maybe we need to vote for using exporter-toolkit/web for web config or not and make a design for how integration it.

I like the idea of embedding webconfig into the config. It is one less configuration to worry about. Let me play around it a bit and get back to you.

Just a nit: ... This PR will be a feat and not a chore https://gist.github.com/qoomon/5dfcdf8eec66a051ecd85625518cfd13#types

I just make some update in code and fix with test cases.

@SamYuan1990 SamYuan1990 changed the title [chore] using exporter-toolkit/web for web config [feat] using exporter-toolkit/web for web config Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Routine tasks or maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants