Skip to content
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

OtelRumConfig().setGlobalAttributes does not query the attribute supplier on every span or log creation #670

Open
jpmuedano opened this issue Oct 30, 2024 · 6 comments

Comments

@jpmuedano
Copy link

Hello :) @LikeTheSalad

Based on the guidance given in issue #625, my understanding is that the OtelRumConfig().setGlobalAttributes {} should query the Supplier<Attributes> on every span or log creation & populate the span attributes without having to explicitly set all attributes to every span through the SpanBuilder.setAllAttributes().

For Example, the OtelRumConfig() :

        val config = OtelRumConfig()
            .setDiskBufferingConfiguration(diskBufferingConfig)
            .setGlobalAttributes { attributesSupplier.get() }

Where the supplier is a simple implementation including a setter:

public interface GlobalAttributeSupplier : Supplier<Attributes> {
    public fun set(attributes: Attributes)
}

public class GlobalAttributeSupplierImpl : GlobalAttributeSupplier {

    private var attributes: Attributes = Attributes.empty()

    public override fun set(attributes: Attributes) {
        this.attributes = attributes
    }

    override fun get(): Attributes = attributes

}

The goal is to be able to update the attribute supplier & add/modify attributes during the application lifetime.

Currently with this approach, I am not seeing the attributes populated on newly created spans, instead I have to include the attributes on every span builder to see the attributes reflected on every trace. I might be missing some other configuration for the global attribute supplier to work?

@LikeTheSalad
Copy link
Contributor

Hi @jpmuedano

It seems like this could be related to a memory visibility issue. I'd recommend annotating your class' private var attributes: Attributes field with @Volatile as shown below:

public class GlobalAttributeSupplierImpl : GlobalAttributeSupplier { 

  @Volatile /* Add this annotation to ensure that your changes to this field are visible from another thread */
  private var attributes: Attributes = Attributes.empty()

  /* ... */
}

Also, I'd recommend to pass your supplier directly to the OtelRumConfig() setter:

// Instead of this
val config = OtelRumConfig(
            .setGlobalAttributes { attributesSupplier.get() }

// Try this
val config = OtelRumConfig(
            .setGlobalAttributes(attributesSupplier)

It's not a big deal, so you can leave it as is if you like, is just that when you use { attributesSupplier.get() } you're essentially wrapping your supplier within another one, which is not needed.

I hope it helps!

@JacekPietrasSpotOn
Copy link

Hello @LikeTheSalad
Checked @Volatile and it didn't help. I see that yesterday @jpmuedano made progress, I will quote him:

By design the OtelRumConfig() accepts a Supplier<Attributes>, but only the AttributeKey<T> and value are dynamic if set upon configuration. With this in mind, I successfully tested the scenario where the globalAttributeKeys: List<String> are set by the client application beforehand. If that is true, the supplier will be queried on every span & log. As long as we update the supplier, the predefined global attributes value becomes dynamic.

@LikeTheSalad
Copy link
Contributor

Can you provide more details on how is everything set up? (not just the OtelRumConfig object). This object needs to be created before initializing the RUM object (which is done via OpenTelemetryRum.builder(app, config).build()). That process is done once, but if you keep your supplier handy after this process, any changes you make to it should be reflected in the logs.

To provide more context, the supplier that is provided in OtelRumConfig ends up in this processor where it's queried on every LogRecordBuilder.emit() call. Now, maybe some other operation made during the RUM initialization could override this behavior, which is why I think we're going to need more details to provide more useful feedback.

@JacekPietrasSpotOn
Copy link

High level structure:

class OpenTelemetryClient(
    application: Application,
) {

    val openTelemetry: OpenTelemetry
    val attributeSupplier = GlobalAttributeSupplierImpl()

    init {
        // definition of attributes
        attributeSupplier.createInitialAttributesWithKey(
            listOf(
                "dynamic_property",
            ),
        )
        val config = OtelRumConfig()
            .setGlobalAttributes { attributeSupplier.get() }
            // ...

        val rumBuilder = OpenTelemetryRum.builder(application, config)
            // ...

        openTelemetry = rumBuilder
            .build()
            .openTelemetry
            .run(GlobalOpenTelemetry::set)
    }

    fun updateDynamicProperty(value: Int) {
        // declaration of attribute
        attributeSupplier.update("dynamic_property", value)
    }
}

And the key operations on attributes:

override fun createInitialAttributesWithKey(keys: List<String>) {
    attributes = Attributes.builder()
        .putAll(keys.associateWith { "" })
        .build()
}
override fun update(key: String, value: String) {
    val tempAttributes: MutableMap<String, Any> = mutableMapOf()
    attributes.forEach { k, v ->
        tempAttributes[k.key] = v
    }
    tempAttributes[key] = value
    set(
        Attributes.builder()
            .putAll(tempAttributes)
            .build(),
    )
}

@LikeTheSalad
Copy link
Contributor

Thank you for these details, I'll take a closer look.

@LikeTheSalad
Copy link
Contributor

LikeTheSalad commented Nov 8, 2024

I've just tested it using our OTel demo app and I couldn't reproduce the issue. I've created this branch with my changes in case you'd like to take a look at how it was all set up. I'll explain the details below:

I replicated your classes GlobalAttributeSupplier and GlobalAttributeSupplierImpl to make my test as similar as possible to your use case, with some minor changes to adjust to our demo app. You can find them here for more details.

I then used those classes when initializing the RUM client here, where I also made the GlobalAttributeSupplierImpl instance available statically.

Once everything was set up, I modified the app's first screen to look like the following:

I've added those 3 buttons, "Send span", "Send log" and "Update global attrs". The action for "Send log" is here and the action for "Update global attrs" is here for reference.

For my tests I used the Elastic stack, although it should all work with any other vendor. If you would like to test it, you should set your vendor's endpoint params here.

First I launched the app and clicked on "Send log", after a couple of seconds this is what I saw:

First list of logs

At the top of the list, we see our log "A log body", when opening its attrs we can find the one that's set up here when initializing the RUM object.

First attr details

It's the dynamic_property attr set to unknown, which is its initial value set here.

Then I clicked on the "Update global attrs" button, which increases this counter and adds its most recent value to the dynamic_property attr.

Then I clicked on "Send log" again, and after a few seconds I opened the newest log at the top:

Logs after change

And found:

After attrs has changed

So I wasn't able to reproduce the issue. Please feel free to try it out to double-check. If you'd like you can also change my branch's code until the issue is reproducible there so that I can take another look.

I hope it helps!

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

No branches or pull requests

3 participants