-
Notifications
You must be signed in to change notification settings - Fork 59
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
integrate spandex ecto #1602
integrate spandex ecto #1602
Conversation
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
defmodule OMG.WatcherInfo.DB.TraceableRepo do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a workaround for spandex-project/spandex_ecto#22. I tried using span
and trace
decorators. They don't record traces from queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even though this works, great job @ayrat555! I think it's super intrusive to our elixir-omg Repo. well... you had to wrap the whole Repo module to make this work.
what if we fork spandex_ecto and bake in the solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I talked with @achiurizo a couple of days ago. He suggested the same. I'll look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonderful!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@InoMurko pr is ready for review
@@ -101,11 +104,12 @@ defmodule OMG.WatcherInfo.DB.TxOutput do | |||
address | |||
|> query_get_utxos() | |||
|> from(limit: ^limit, offset: ^offset) | |||
|> Repo.all() | |||
|> DB.Repo.all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're trying to avoid namespacing outside the module header
|> DB.Repo.all() | |
|> Repo.all() |
Repo.all(query) | ||
|> Enum.map(fn {currency, amount} -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a good oppurtunity to avoid a single pipe
query
|> Repo.all()
|> Enum.map(fn {currency, amount} ->
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@InoMurko yes, I thought about it. But I decided now to include additional changes. Also, there is a credo check for this stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we can't use it because we have too many of them :D
so we do this:
https://github.com/omgnetwork/elixir-omg/blob/master/.circleci/config.yml#L181-L192
Overview
This PR adds ecto_spandex integration. Traces will look like
https://app.datadoghq.com/apm/resource/ecto/OMG.WatcherInfo.ReleaseTasks.EthereumTasks.AddEthereumHeightToEthEvents.run_0/bc4ebb28a19b26f?end=1593420747117&env=local-development&index=apm-search&paused=false&query=env%3Alocal-development%20service%3Aecto%20operation_name%3AOMG.WatcherInfo.ReleaseTasks.EthereumTasks.AddEthereumHeightToEthEvents.run_0%20resource_name%3A%22OMG.WatcherInfo.ReleaseTasks.EthereumTasks.AddEthereumHeightToEthEvents.run%2F0%22&start=1593406347117&event=AQAAAXL-yXqpCsROoQAAAABBWEwreVkxMjJGcWNmMWpNTWVNMQ&traceID=3852440086353038908&spanID=4515663423801199690