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

549 additional application metrics 2 #851

Merged
merged 7 commits into from Jul 25, 2019

Conversation

InoMurko
Copy link
Contributor

@InoMurko InoMurko commented Jul 15, 2019

📋 Add associated issues, tickets, docs URL here.

Overview

Migration from AppSignal to Datadog with Spandex, Spandex Ecto and Spandex Phoenix with supporting lib VmStats.

Changes

  • Removed AppSignal completely
  • Support to exclude :rocksdb as dependency, useful for local builds because it taking ages to compile.
  • Swapped custom function annotations provided by Appsignal with SpandexDecorators and added example usage pattern for syncing procedure that happens in EthereumEventListener. Will be adding more as we proceed.
  • to ensure telemetry handlers are attached they're part of startup procedure https://hexdocs.pm/elixir/Application.html#c:start_phase/3
  • Removed Recorder process and introduced telemetry handlers (*...Measure modules - they should be understood as an extension (like Core modules) to the parent process module) for named processes.
  • removed increment_metrics_counter functions in controller and will later add them as a Plug.
  • moved dependencies that we altered into OmiseGo repo

Testing

No tests removed or features changed so same testing would apply.

@coveralls
Copy link

coveralls commented Jul 17, 2019

Coverage Status

Coverage increased (+0.5%) to 85.026% when pulling cc192f9 on 549-Additional_application_metrics_2 into 117d556 on master.

@InoMurko InoMurko force-pushed the 549-Additional_application_metrics_2 branch 8 times, most recently from 149624f to 3bf5c22 Compare July 22, 2019 14:29
@InoMurko InoMurko force-pushed the 549-Additional_application_metrics_2 branch 5 times, most recently from a5b35a3 to e6380ec Compare July 24, 2019 20:27
@InoMurko InoMurko marked this pull request as ready for review July 24, 2019 20:48
@InoMurko InoMurko force-pushed the 549-Additional_application_metrics_2 branch from e6380ec to 58df8ad Compare July 24, 2019 20:57
@InoMurko InoMurko force-pushed the 549-Additional_application_metrics_2 branch from 58df8ad to 51f4fa4 Compare July 24, 2019 21:00
@InoMurko
Copy link
Contributor Author

Some of the things we're able to gather now via APM.
Screen Shot 2019-07-24 at 23 04 02
Screen Shot 2019-07-24 at 23 03 31
Screen Shot 2019-07-24 at 23 02 13
Screen Shot 2019-07-24 at 23 01 57

Copy link
Contributor

@chrishunt chrishunt left a comment

Choose a reason for hiding this comment

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

This is great @InoMurko! Dropped a few comments.

mix.lock Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.dev.yml Outdated Show resolved Hide resolved
apps/omg_watcher/lib/omg_watcher/api/utxo.ex Outdated Show resolved Hide resolved
apps/omg_watcher/lib/omg_watcher/block_getter.ex Outdated Show resolved Hide resolved
apps/omg_watcher/lib/omg_watcher/api/transaction.ex Outdated Show resolved Hide resolved
apps/omg_watcher/lib/omg_watcher/utxo_exit/core.ex Outdated Show resolved Hide resolved
apps/omg_child_chain/lib/child_chain.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@pdobacz pdobacz left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of non-blocking comments added

apps/omg/lib/omg/ethereum_event_listener.ex Outdated Show resolved Hide resolved
apps/omg/lib/omg/state.ex Show resolved Hide resolved
apps/omg_db/test/fixtures.exs Outdated Show resolved Hide resolved
@@ -138,6 +144,7 @@ defmodule OMG.EthereumEventListener.Core do

height_to_check_in = get_height_to_check_in(new_state)
db_update = [{:put, update_key, height_to_check_in}]
:ok = :telemetry.execute([:process, __MODULE__], %{events: events}, state)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call this outside of .Core so that it remains side-effect free?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm... what causes the side effect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't talking to :telemetry one?

Sorry if I'm having false assumptions here, but I'm guessing this talks to some dependency to do sth. .Core.xyz functions are written with the intent of remaining pure.

Unless absolutely necessary we avoid doing calling external stuff in .Core.xyz, I think the only exception was made for _ = Logger... calls, but only in a few cases.

events and state are available outside, so the move seems to be an easy one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This event gets forwarded to the dedicated Ethereum event listener handler located in module OMG.EthereumEventListener.Measure

I don't quite understand what you mean by side effect though indeed the change isn't big, but would like to understand where keeping .Core.xyz pure comes from.

Copy link
Contributor

Choose a reason for hiding this comment

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

would like to understand where keeping .Core.xyz pure comes from.

Sure thing! It comes from the approach adopted, so segregate logic into such Core modules, with no side effects/side causes. These modules would hold pure functions (same output for same input etc.). Then they can be robustly/quickly tested in unit tests and also I think they are easier to reason about, in general, being just these reliable chunks of logic.

apps/omg/lib/omg/state.ex Show resolved Hide resolved
apps/omg_db/lib/omg_db/leveldb/core.ex Outdated Show resolved Hide resolved
@InoMurko InoMurko force-pushed the 549-Additional_application_metrics_2 branch from 3d155c8 to 35b1425 Compare July 25, 2019 14:09
@InoMurko
Copy link
Contributor Author

A previous commit removes logging and replaces it with datadog trace
dbcd522#diff-5fcd99569a2ce28c62745940476ced06R233

looks liek this:

Screen Shot 2019-07-25 at 16 30 08

Copy link
Contributor

@chrishunt chrishunt left a comment

Choose a reason for hiding this comment

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

🚀

@InoMurko InoMurko mentioned this pull request Jul 25, 2019
@InoMurko InoMurko merged commit 3b42607 into master Jul 25, 2019
@InoMurko InoMurko deleted the 549-Additional_application_metrics_2 branch July 25, 2019 15:31
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.

None yet

6 participants