Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

add support for ioredis #32

Closed
wants to merge 1 commit into from
Closed

add support for ioredis #32

wants to merge 1 commit into from

Conversation

schmod
Copy link
Contributor

@schmod schmod commented Jan 25, 2018

Adds instrumentation for the ioredis Redis client.

Closes #30.

@bripkens
Copy link
Contributor

Thank you for this great PR @schmod ❤️ ! I manually merged it with some changes to adapt it to our guidelines (admittedly not documented so you couldn't have known). Commit: 327750e

Thanks again!

@bripkens bripkens closed this Jan 26, 2018
@bripkens
Copy link
Contributor

Once the tests are executed & green on travis, I will release your changes.

@schmod
Copy link
Contributor Author

schmod commented Jan 26, 2018

@bripkens I think you conflated multi and pipeline in your commit -- ioredis has both.

@bripkens
Copy link
Contributor

Ah, I was wondering why you added this and why the tests were still green after I removed this condition. We will need additional tests for pipeline and then also adapt the implementation accordingly.

@schmod
Copy link
Contributor Author

schmod commented Jan 26, 2018 •
edited
Loading

You can probably just reuse/adapt the multi test -- from our perspective, the semantics are exactly the same.

@bripkens
Copy link
Contributor

The semantics are slightly different as I understand it. multi defines a Redis transaction. So one failed command will fail the entire transaction. Because of this, it makes sense to model a transaction as a single span.

I understand pipeline as a why to optimize predominantly for network latency. Because of this, I think that commands should be handled differently when pipelined. More specifically, I do not think that they should be aggregated into a single span (as each command has its own error state).

@bripkens
Copy link
Contributor

Alternatively, pipeline commands could be modeled as a single span, but the batch and error count semantics (ec field in the span) must be respected.

@bripkens
Copy link
Contributor

I will refrain from releasing this for now. We need at least a test to verify that pipelining still works.

@bripkens
Copy link
Contributor

Taking of this now.

@bripkens bripkens mentioned this pull request Jan 26, 2018
Merged
@bripkens
Copy link
Contributor

Follow up in: #34

willianpc added a commit that referenced this pull request Apr 6, 2021
@willianpc
# This is a combination of 56 commits.
2768bb6 
# This is the 1st commit message:

Added basic test apps

# This is the commit message #2:

improved test app a little bit

# This is the commit message #3:

more refactoring; wip: process, concurrency named process in test app

# This is the commit message #4:

added consumer as process

# This is the commit message #5:

wip: in the middle of another refactoring

# This is the commit message #6:

test app covers all 12 cases

# This is the commit message #7:

started instrumentation

# This is the commit message #8:

wip: instrumenting add() function

# This is the commit message #9:

wip: dealing with repeatable jobs

# This is the commit message #10:

Using repeat with limit. avoid using cron

# This is the commit message #11:

added bulk job sending

# This is the commit message #12:

small refactoring in sender test app

# This is the commit message #13:

exit span instrumented for single and repeatable jobs

# This is the commit message #14:

wip: bulk immediate jobs

[skip ci]

# This is the commit message #15:

wip: testing the instrumentation of bulk > send job

[skip ci]

# This is the commit message #16:

complete instrumentation of exit spans

# This is the commit message #17:

docs(changelog): prepare release 1.117.0

[skip ci]

# This is the commit message #18:

v1.117.0

# This is the commit message #19:

fix: bypass native addon loading in worker threads

Co-authored-by: Willian Carvalho <willian.carvalho@instana.com>
# This is the commit message #20:

docs(changelog): prepare release 1.117.1

[skip ci]

# This is the commit message #21:

v1.117.1

# This is the commit message #22:

test(tracing): make batching test less flaky

# This is the commit message #23:

chore: add script for splitting CI output according to package

[skip ci]

# This is the commit message #24:

fix: update to shimmer@1.2.1 to be able to patch async functions

Specifically, that version includes this fix:
othiym23/shimmer@ec15ba2

# This is the commit message #25:

test(tracing): fix path for custom tags

# This is the commit message #26:

wip: added ENTRY span instrumentation

# This is the commit message #27:

renamed child process.js; removed extra execTime param from /POST; added client HTTP call

# This is the commit message #28:

immediate and repeatable jobs instrumented correctly

[skip ci]

# This is the commit message #29:

small refactoring/cleanup; [skip ci]

# This is the commit message #30:

introduced tests

[skip ci]

# This is the commit message #31:

Included test for Callback x Bulk [skip ci]

# This is the commit message #32:

wip: attempting to instrument master.js [skip ci]

# This is the commit message #33:

instrument child process with an extra entry span

# This is the commit message #34:

experimental: activate tracing immediately, without waiting for connection to agent

# This is the commit message #35:

send spans directly from Bull child process workers and wait for them to be offloaded

# This is the commit message #36:

fix linting issues

# This is the commit message #37:

Updating agent uuid from env var info

[skip ci]

# This is the commit message #38:

fixed PID casting to string [skip ci]

# This is the commit message #39:

no more IPC, write to a file instead

# This is the commit message #40:

attach trace context to each individual IPC message instead of env var at process start

# This is the commit message #41:

avoid duplicated bull entry span

# This is the commit message #42:

fixed naming convention; replaced fs/promises by fs; guarantee that opts object exists

# This is the commit message #43:

added disabled trace tests

# This is the commit message #44:

lift the requirement for applying span.disableAutoEnd/span.end()

# This is the commit message #45:

Running tests for trace disabled and suppressed

# This is the commit message #46:

all instana data is removed before customer gets the processed job data

# This is the commit message #47:

Added test to make sure no Instana data is left in Job data

# This is the commit message #48:

Added withError cases

# This is the commit message #49:

Skipping tests for Node version < 10

# This is the commit message #50:

fix(opentracing): default to type entry when no parent is referenced

# This is the commit message #51:

docs(changelog): prepare release 1.117.2

[skip ci]

# This is the commit message #52:

v1.117.2

# This is the commit message #53:

fix(metrics): register gc stats loader listener immediately

This fixes an issue where GC metrics would not be available when the
dependency gcstats.js was already present in node_modules and the
activation of the gc metrics module would happen too late.

# This is the commit message #54:

docs(changelog): prepare release 1.117.3

[skip ci]

# This is the commit message #55:

v1.117.3

# This is the commit message #56:

docs(contributing): document the need to install PostgreSQL headers

(Only required for local development.)

Plus: Additional info about rate limited OTP when publishing.

[skip ci]
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@schmod @bripkens