feat: Add LangfuseConnector secure key management and serialization #1287
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
added
integration:langfuse
type:documentation
|
Let's ask @ArzelaAscoIi for some input. @ArzelaAscoIi please see this comment |
julian-risch
requested changes
Jan 17, 2025
There was a problem hiding this comment.
Looks quite good to me already. My change requests are only about the tests.
In addition to my inline comments about monkeypatch.setenv for unit tests, let's add a unit test to check backwards compatibility of the serialization and use
monkeypatch.delenv("LANGFUSE_SECRET_KEY", raising=False)
monkeypatch.delenv("LANGFUSE_PUBLIC_KEY", raising=False)to make sure env variables are not set for this test.
|
|
||
|
|
||
| def test_pipeline_serialization(): | ||
| """Test that a pipeline with secrets can be properly serialized and deserialized""" |
There was a problem hiding this comment.
If we expect the envs to be set for this unit test, let's use monkeypatch.setenv, for OPENAI_API_KEY too.
There was a problem hiding this comment.
Yes here it makes sense as it is unit test
julian-risch
requested changes
Jan 17, 2025
Amnah199
pushed a commit
that referenced
this pull request
Jan 28, 2025
…1287) * LangfuseConnector: add secret_key and public_key init params * Update tests * Linting * Add serde test * Lint * PR feedback * PR feedback
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.
Why:
Enhances the
LangfuseConnectorby introducing secure handling of API keys through environment variables, improving security and flexibility in configuration.What:
public_keyandsecret_keyparameters to the LangfuseConnector's constructor, utilizing environment variables for secure access.to_dictandfrom_dictmethods for serialization and deserialization of the connector, facilitating easier configuration management.How can it be used:
The
LangfuseConnectorcan now be initialized with secure keys retrieved from environment variables, enhancing security in production environments. Here's how it looks:How did you test it:
Integration tests were updated to validate the new key management features by creating pipeline fixtures that utilize both environment variables and Secret objects. This ensures that the connector behaves as expected under different configurations.
Notes for the reviewer:
Please pay special attention to the new serialization methods and how the environment variables are handled in tests. Ensure that the key management complies with best security practices and that the changes integrate seamlessly with existing features.