-
Notifications
You must be signed in to change notification settings - Fork 3
traceparent work #41
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
traceparent work #41
Conversation
|
added settings work |
|
There are errors reported on the manage extensions page: And there are compile warnings: I am not sure what these mean. any insights appreciated. |
|
I found and got rid of the errors in the last commit. It seems to work correctly overall with one exception: it requires selecting a sandbox or enabling/disabling to get a change to the trace parent header to start being injected. |
|
Fixed reactivity in last commit |
|
I haven't test it, but one thing we have to consider is how user will access that configuration panel and also how we plan to obfuscate the API configuration. So probably we will have to add the setting button again, and add under settings the shortcut to access the API configuration. FYI @foxish |
Can this be considered in a separate PR? |
davixcky
left a comment
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.
LGTM, tested that is adding the traceparent.
Note that I didn't test if the routing key is being propagated, but just that is being set
src/service-worker.ts
Outdated
|
|
||
| export const getHeaders = (extraHeaders: string[] | undefined, traceparentHeader: string | undefined): Record<string, Header> => { | ||
|
|
||
| const traceparentObj: Record<string, Header> = traceparentHeader ? { "traceparent": {value: traceparentHeader, kind: "always" }} : {}; |
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 like this since is not really the traceparentObj but a map of headers
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 in 37272a9
src/service-worker.ts
Outdated
| chrome.runtime.onStartup.addListener(updateDynamicRules); | ||
| chrome.storage.onChanged.addListener((changes, areaName) => { | ||
| if (areaName === "local" && (changes[ROUTING_KEY] || changes[ENABLED_KEY])) { | ||
| if (areaName === "local" && (changes[ROUTING_KEY] || changes[ENABLED_KEY] || changes[StorageKey.TraceparentHeader])) { |
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.
If you could update existing keys, we will awesome too
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.
Sorry, don't understand concretely what you suggest. Could you explain in more detail?
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.
Do you mean use StorageKey.Enabled instead of ENABLED and the like? I did that in 37272a9
|
Further testing showed that for some reason I don't understand, the DEFAULT_TRACEPARENT_HEADER wasn't effectively the default, rather I added an explicit check for undefined and use the default at the updateDynamicHeaders function body, and that fixed it. |
|
Even further testing revealed that there is odd behavior w.r.t. the default value and empty string. When setting it to the empty string, it behaves as intended and does not add a traceparent header. However, when re-opening the settings pop-up, it comes back as the default. Not sure what we should do about that, I was hoping to avoid requiring an extra checkbox for whether to inject or not. Thoughts? |
Screen.Recording.2025-03-03.at.4.14.43.PM.movCreated this settings panel to disable/enable the traceparent header with a switch instead of a empty value which cause some confussion Thoughts? cc: @signadot/engineering |
This seems to be the right behavior to me. @scott-cotton can you please double check based on the video? I'll do a full code review asap. |
Sorry for the delay, the video looks great! |
<img width="75" alt="Screenshot 2025-02-24 at 7 21 39 AM" src="https://github.com/user-attachments/assets/bb5e5d51-169b-4e20-ac25-1628942a1dfb" /> <img width="483" alt="Screenshot 2025-02-24 at 7 21 33 AM" src="https://github.com/user-attachments/assets/fe1da67d-0144-4f0e-9177-610fecf37611" /> <img width="490" alt="Screenshot 2025-02-24 at 7 21 26 AM" src="https://github.com/user-attachments/assets/bb45cb7d-05a7-4782-9ae6-a984b97f6e6d" /> <img width="498" alt="Screenshot 2025-02-24 at 7 21 21 AM" src="https://github.com/user-attachments/assets/eefe2beb-eaf0-4ca2-be40-ea9155b706d8" /> <img width="478" alt="Screenshot 2025-02-24 at 7 21 14 AM" src="https://github.com/user-attachments/assets/e3db92f1-9ec9-45d8-864d-efcceccc8f38" /> **Home** - Keep footer in the bottom - Add no-state when no sbx/rg selected **Layout** - Add context for routing (custom router) - Add button next to the toggle **Settings** - Hide extra (signadot internal specific) settings
|
@foxish @scott-cotton this is ready for review since i added the toggle and minor tweaks. @foxish should i merge this to main and test from there? |
Awesome, thanks much. I'll leave the review to @foxish as I'd just rubber stamp it. |
foxish
left a comment
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.
LGTM, let's test with the extension built from main @davixcky for a week or two before releasing.
wanted to put this up as a draft to get review and ask questions along the way, since I'm new to this.
edit: this seems to working ok to me, marking ready for review