Move oboe sampler and inbounds metrics to solarwinds-apm package #443
Conversation
packages/test/src/index.ts
Outdated
| beforeEach(async function () { | ||
| const CURRENT_RETRY = "currentRetry" | ||
| const currentRetry = this.currentTest?.[CURRENT_RETRY]() | ||
| if (currentRetry) { | ||
| await setTimeout(currentRetry * 1000) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Introduce a timeout between failed test retries (useful for sampler integration tests where we want to get a successful sampling decision but are hitting a rate limit)
| function withoutUndefinedProperties( | ||
| source: object, | ||
| deep: boolean, | ||
| copy = true, | ||
| ): object { | ||
| const object = copy | ||
| ? (Object.create( | ||
| Object.getPrototypeOf(source) as object | null, | ||
| Object.getOwnPropertyDescriptors(source), | ||
| ) as object) | ||
| : source | ||
|
|
||
| for (const key of Reflect.ownKeys(object)) { | ||
| const descriptor = Reflect.getOwnPropertyDescriptor(object, key)! | ||
| if ( | ||
| descriptor.enumerable && | ||
| descriptor.configurable && | ||
| descriptor.value === undefined | ||
| ) { | ||
| Reflect.deleteProperty(copy, key) | ||
| Reflect.deleteProperty(object, key) | ||
| } else if (deep && isBasicObject(descriptor.value)) { | ||
| withoutUndefinedProperties(descriptor.value, true, false) | ||
| } | ||
| } | ||
|
|
||
| return copy | ||
| return object | ||
| } |
There was a problem hiding this comment.
This is pretty nasty code because it needs to be to deal with JS weirdness but essentially I just changed it to be recursive if desired.
| Assertion.overwriteMethod("equal", loosely) | ||
| Assertion.overwriteMethod("equals", loosely) | ||
| Assertion.overwriteChainableMethod("include", loosely, identity) | ||
| Assertion.overwriteChainableMethod("includes", loosely, identity) | ||
| Assertion.overwriteChainableMethod("contain", loosely, identity) | ||
| Assertion.overwriteChainableMethod("contains", loosely, identity) |
There was a problem hiding this comment.
Main change is supporting the loosely option for more types of tests
There was a problem hiding this comment.
Wow 💯 on the added tests, thanks @raphael-theriault-swi! I left a couple minor comments, feel free to address in follow-on PR.
| BucketCapacity: 100, | ||
| BucketRate: 500, | ||
| "custom-foo": "custom-bar", | ||
| SWKeys: "sw", |
There was a problem hiding this comment.
Would the TriggeredTrace: true attribute also be set here and if so, can the test also check?
Also, pretty minor, the SampleRate and SampleSource ideally would be unset in the case of a triggered trace.
There was a problem hiding this comment.
I was not aware of the TriggeredTrace attribute, I think it was never implemented here woops. I'll add those changes, I also need to change some of the stable HTTP semconv handling because url.full isn't actually present on the spans
There was a problem hiding this comment.
Updated to add the missing TriggeredTrace attribute and also SampleSource which was also missing. Also removed any logic related to local sample rate since that made implementing sample sources easier and it's not used anywhere anyway.
This moves the oboe-based sampler and inbound metrics span processors next to the rest of the new code. Most of their implementation is just lifted from the existing ones but it was simplified wherever possible by using some of the newly introduced code.
They are now pretty well tested (unlike previously) and I plan on including some more integration tests in future PRs along the reporter and exporters.
There were some improvements to the testing helpers too to make some of the tests less flaky.