feat: add library context field #76
Merged
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
liuyang1520
requested review from
justin-fiedler,
yuhao900914,
qingzhuozhen and
bohan-amplitude
liuyang1520
commented
Aug 18, 2022
| @@ -24,6 +24,7 @@ public class Amplitude { | |||
| private Object eventQueueLock = new Object(); | |||
| private Plan plan; | |||
| private long flushTimeout; | |||
| private String libraryContext; | |||
There was a problem hiding this comment.
I found that adding a new middleware to this repo might be a little overkilled, e.g.:
public class ContextMiddleware implements Middleware {
public String libraryContext;
public ContextMiddleware(String libraryContext) {
this.libraryContext = libraryContext;
}
@Override
public void run(MiddlewarePayload payload, MiddlewareNext next) {
if (payload.event != null) {
payload.event.libraryContext = this.libraryContext;
}
next.run(payload);
}
}
There was a problem hiding this comment.
What'll be your suggestion?
There was a problem hiding this comment.
Oh, my preference would be the change in this PR.
- The property can be globally set (like
plan), doesn't need to be applied per event - Lightweight and minimum changes, adding a new middleware and exporting it in package only for this case seems overkill to me
For the Python (new generation SDk) change: https://github.com/amplitude/Amplitude-Python/pull/33/files. Similarly, exposing a way to set it globally, as Python puts the library in ContextPlugin, we can put in the similar place, while keeping the changes minimum and lightweight.
qingzhuozhen
approved these changes
Aug 18, 2022
bohan-amplitude
approved these changes
Aug 18, 2022
justin-fiedler
approved these changes
Aug 18, 2022
liuyang1520
added a commit
that referenced
this pull request
Aug 25, 2022
This reverts commit c874396.
liuyang1520
added a commit
that referenced
this pull request
Aug 25, 2022
justin-fiedler
pushed a commit
that referenced
this pull request
Jun 20, 2023
# [1.11.0](v1.10.0...v1.11.0) (2023-06-20) ### Bug Fixes * fix failed unit tests due to async methods ([#74](#74)) ([026c3cc](026c3cc)) * handle non-json response bodies and retry on 5xx errors. ([#85](#85)) ([1d4567e](1d4567e)) ### Features * add ingestion_metadata field ([#78](#78)) ([727bb51](727bb51)) * add library context field ([#76](#76)) ([c874396](c874396)) ### Reverts * Revert "feat: add library context field (#76)" (#77) ([7d9e474](7d9e474)), closes [#76](#76) [#77](#77)
|
🎉 This PR is included in version 1.11.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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.
Summary
Add this option in the configuration, which can be used in the library wrapper/code generation/dynamic loading cases, for setting the
library_contextinformation.Checklist