Skip to content

Conversation

@Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Jun 2, 2020

This also exposes a new global with_integration function which is also
used in existing integrations.

Integrations are still run in order, but later integrations override
former instances, and should run `setup` and be returned by
`with_integration`.

This also exposes a new global `with_integration` function which is also
used in existing integrations.
@Swatinem Swatinem requested a review from jan-auer June 2, 2020 16:43
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you deduplicate integrations - is this unified behavior? Spontaneously, I'd argue that it might make perfect sense to initialize an integration twice with different settings.

Of course, with_integration would then only return one of them, but potentially there is no need to access such integrations later on.

@Swatinem
Copy link
Contributor Author

Swatinem commented Jun 3, 2020

Of course, with_integration would then only return one of them, but potentially there is no need to access such integrations later on.

One of the obvious usecases is to customize the default PanicIntegration with a custom extractor. Since that relies on the with_integration ordering, it would never use the custom extractor, unless you explicitly use default_integrations: false, and manually add all the defaults.

On the other hand, it totally makes sense to have multiple instances of event-processor-like integrations.

I think I will revert the unique-ness check, and rather better document this case, and also the default integrations.

@jan-auer
Copy link
Member

jan-auer commented Jun 3, 2020

unless you explicitly use default_integrations: false, and manually add all the defaults

But isn't that exactly what the unified spec says? My understanding was that you need to disable default integrations if you want to customize one of them.

@Swatinem Swatinem changed the title fix: Make sure we de-duplicate Integrations fix: Better document default integrations Jun 3, 2020
@Swatinem
Copy link
Contributor Author

Swatinem commented Jun 3, 2020

But isn't that exactly what the unified spec says?

Hm, it doesn’t really say anything about integrations :-D

Anyhow, I added some more docs here.

@Swatinem Swatinem merged commit 0a361de into master Jun 4, 2020
@Swatinem Swatinem deleted the fix/unique-integrations branch June 4, 2020 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants