appsignal / appsignal-elixir

🟪 AppSignal for Elixir package
https://www.appsignal.com/elixir
MIT License
285 stars 82 forks source link

2.0 beta.8 doesn't respect sample data #580

Closed hubertlepicki closed 4 years ago

hubertlepicki commented 4 years ago

I have just added a semi-related issue here https://github.com/appsignal/appsignal-elixir/issues/579

but in addition the the above, I am experiencing issues when i try to set sample data on span. Basically, what I want to achieve is to decorate my samples, when error happens, with params, environment etc., the way I could do (and the way Appsignal.Plug) was doing in version 1.x. I want to see this:

appsignal2 and appsignal3

I figured out, this should be doable with Appsignal.Span.set_attribute and Appsignal.Span.set_sample_data.

While set_attribute seems to work just fine, and I can see these attributes I set as tags, I can't get set_sample_data to show up on my dashboard -> errors -> sample.

I looked at the new Plug code and it seems to be doing precisely what I do: https://github.com/appsignal/appsignal-elixir-plug/blob/main/lib/appsignal_plug.ex#L139

My code that I use to report errors:

Appsignal.instrument fn span -> 
  Appsignal.Span.set_namespace(span, "background") 
  |> Appsignal.Span.set_sample_data("params", %{my_param: "my_value"}) 
  |> Appsignal.Span.set_attribute("foo", "bar") 
  |> Appsignal.Span.set_name("Exq/error") 
  |> Appsignal.Span.add_error(:error, "MyError", []) 
  |> Appsignal.Tracer.close_span()
end

I expected to see "Parameters" section there but Id on't see it. Is there something magical I have to do to make it appear?

jeffkreeftmeijer commented 4 years ago

Hey @hubertlepicki,

Thanks for reporting this! Is this running in a Plug app? If so, the params you're setting might be overridden when we try to set the params we get reported from Plug at the end of the request, which overwrites the params you've already set. If we receive an empty map, the current version will drop your already set params.

hubertlepicki commented 4 years ago

Ah. We are running an umbrella app and in the "master proxy" pattern as this is running on Gigalixir. It's a Phoenix app proxying requests to other Phoenix endpoints.

But in my code I am actually tring to set params that are being send to exq background job. So the issue is not setting params up on web requests, but setting them up for background tasks.

jeffkreeftmeijer commented 4 years ago

Aha, thanks for getting back to me. Could you post a code sample, so I can try to reproduce your setup to see what's going on? I believe the issue has something to do with the app outside of the code sample you've already posted here.

If you prefer, you can also do that via support@appsignal.com, I'll pick up up there if you do.

jeffkreeftmeijer commented 4 years ago

A quick update from our side; we're able to reproduce this issue, and we're investigating where the parameters drop from the samples. In our testing, we were able to verify that the params are correctly added to the spans sent from the integration and registered in the agent, so this issue is most likely not caused by the Elixir integration itself.

However, this issue blocks the release of AppSignal for Elixir 2.0, and we'll be sure to keep you posted in this issue. Thanks @hubertlepicki, for reporting this and #579, for which the above is also true.

PS: When using Appsignal.instrument/1, there's no need to call Appsignal.Tracer.close_span/1 to close the current span. The instrument/1 function will do that after executing the passed fun. Removing that doesn't solve this issue, but will prevent warnings from being thrown in the AppSignal log.

hubertlepicki commented 4 years ago

Thanks for the update and for the tip on Appsignal.Tracer.close_span1/. Kind regards Hubert

On Fri, Sep 25, 2020 at 10:51 AM Jeff Kreeftmeijer notifications@github.com wrote:

A quick update from our side; we're able to reproduce this issue, and we're investigating where the parameters drop from the samples. In our testing, we were able to verify that the params are correctly added to the spans sent from the integration and registered in the agent, so this issue is most likely not caused by the Elixir integration itself.

However, this issue blocks the release of AppSignal for Elixir 2.0, and we'll be sure to keep you posted in this issue. Thanks @hubertlepicki https://github.com/hubertlepicki, for reporting this and #579 https://github.com/appsignal/appsignal-elixir/issues/579, for which the above is also true.

PS: When using Appsignal.instrument/1, there's no need to call Appsignal.Tracer.close_span/1 to close the current span. The instrument/1 function will do that after executing the passed fun. Removing that doesn't solve this issue, but will prevent warnings from being thrown in the AppSignal log.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/appsignal/appsignal-elixir/issues/580#issuecomment-698808545, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAID6XWLEM2COQ5CA3PVWDSHRKX7ANCNFSM4QLSH6TQ .

-- Hubert Łępicki phone: +48 694 161 264 email/Google+: hubert.lepicki@amberbit.com Skype: hubert.lepicki

AmberBit Sp. z o. o. ul. Hetmańska 42 lok. 205 15-727 Białystok

AmberBit Sp. z o. o. jest wpisana do Rejestru Przedsiębiorców Krajowego Rejestru Sądowego prowadzonego przez Sąd Rejonowy w Białymstoku, XII Wydział Gospodarczy Krajowego Rejestru Sądowego. Kapitał zakładowy 20 000,00 zł opłacony w całości. EU VAT: PL5423228204, NIP: 5423228204, KRS: 0000439100, REGON: 200741641

jeffkreeftmeijer commented 4 years ago

Hey @hubertlepicki,

We've been able to identify this issue, and the one described in #579, and we've pushed some fixes to our processing pipeline. We've also had to make some changes to the agent shipped with the current Elixir beta's, so we've released 2.0.0-beta.10 last week.

Could you try updating your dependency on :appsignal (mix deps.update appsignal), and try the test case you described above again? You should now receive all errors you send this way.

Thanks once more for taking the time to test our beta versions and reporting this issue!

hubertlepicki commented 4 years ago

I am testing this now.

hubertlepicki commented 4 years ago

@jeffkreeftmeijer this appears to be resolved. Thanks!