[SDK-3975] Strip logging from modular variant of the SDK #1536
Conversation
This file contains 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
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1536/typedoc
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
24e32c2 to
740c29b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
740c29b to
06c0a37
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1536/typedoc
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
06c0a37 to
32dd188
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
32dd188 to
72875fd
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1536/typedoc
VeskeR
requested changes
Dec 8, 2023
There was a problem hiding this comment.
I really like the solution of actually building the syntax tree using babel, and then stripping the logs based on found expression.
This is way more reliable than relying on regular expressions to parse strings to find lines of code that are related to the Logger calls - our initial idea.
Also check my code comments
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
72875fd to
d0f28a2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
d0f28a2 to
0f1fe78
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1536/typedoc
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
0f1fe78 to
f23eb92
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
VeskeR
approved these changes
Dec 11, 2023
There was a problem hiding this comment.
LGTM!
Let's wait for @owenpearson to take a look at logs that are preserved.
owenpearson
requested changes
Jan 15, 2024
There was a problem hiding this comment.
I think we ought to add some logging to the platform http clients in order to properly add tracing for http requests - there are a few places where we call methods directly on the http client rather than using the Resource methods so currently this wont catch anything (same kinda goes for protocol.send because we call transport.send directly in one place but that will change once we remove upgrade so it's fine to keep it as is).
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
f23eb92 to
c0eca61
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
c0eca61 to
0dda975
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
0dda975 to
b0001e2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1536/features
github-actions
bot
temporarily deployed
to
staging/pull/1536/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1536/bundle-report
Need this in order to use esbuild plugins.
In order to reduce bundle size further, we’ve decided to strip all logging from the modular variant of the SDK, except for errors and certain network events. (We also considered providing a separate tree-shakable module with all of this logging code so that a user of the modular variant of the library can opt in to it, but decided against it for now; we might add it in later. This does mean that there is currently no version of the SDK that allows you to use both deltas and verbose logging on web.) I couldn’t find any out-of-the-box esbuild functionality that let us do this. The only stuff I could find related to stripping code was: - the `pure` option, but that code only gets stripped if you minify the code (and even in that case I couldn’t actually get it to be stripped, perhaps would have been able to with further trying though), but minifying our generated modules bundle causes the bundle size of those who use it (as tested by our modulereport script) to increase considerably (for reasons I’m not sure of) - the `drop` option, but that only lets you remove calls to `console` or `debugger` So instead I’ve implemented it as an esbuild plugin. Resolves #1526.
lawrence-forooghian
force-pushed
the
1526-strip-logging
branch
from
b0001e2 to
c5935d8
Compare
lawrence-forooghian
changed the base branch from
integration/v2
to
improve-http-logging
Sure — I've done this in #1581, on top of which the current PR is now based. |
|
@VeskeR would you mind resolving your comments please? |
|
@lawrence-forooghian Resolved |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Note: This PR is based on top of #1581; please review that one first.
In order to reduce bundle size further, we’ve decided to strip all logging from the modular variant of the SDK, except for errors and certain network events. See commit messages for more details.
I'd welcome feedback on whether the logs that I’ve chosen to preserve (i.e. the calls to
logActionNoStrip) are the right ones.Resolves #1526.