Skip to content

[CA-39] Add AppSignal instrumentation (v0.3.0) #19

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.

Sign up for GitHub

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

Jump to bottom
Merged
HNKhoi2410 merged 4 commits into from
Nov 20, 2025
Merged

[CA-39] Add AppSignal instrumentation (v0.3.0) #19

HNKhoi2410 merged 4 commits into from
Nov 20, 2025

Conversation

@HNKhoi2410
Copy link
Contributor

@HNKhoi2410 HNKhoi2410 commented Nov 14, 2025 •
edited
Loading

Summary

  • Add transaction tracking for AppSignal (>= 1.3.0)
  • Support simultaneous instrumentation in both APM tools with nested transactions
  • Add observability configuration options (appsignal_enabled)
  • Bump version to 0.3.0

Changes

  • New Feature: APM transaction tracking for use_cache method
    • AppSignal: Uses monitor_transaction to add method to trace stacks
    • Both tools can be enabled simultaneously for comprehensive observability
  • Configuration: New config.observability settings for enabling APM tools
  • Dependencies: Added optional gems to Gemfile for testing (appsignal)
  • Tests: Comprehensive unit tests covering all APM instrumentation scenarios (11 tests)
  • Documentation: Updated README with usage examples and configuration details

Testing

  • ✅ All 44 tests passing
  • ✅ Rubocop clean
  • ✅ Covers scenarios: AppSignal only, Sentry only, both enabled, neither enabled, error handling

Backward Compatibility

  • ✅ All new features are optional (defaults to false)
  • ✅ No breaking changes to existing API
  • ✅ Users without APM tools configured see no difference

🤖 Generated with Claude Code

@HNKhoi2410 @claude
Add AppSignal and Sentry APM instrumentation (v0.3.0)
fe996d4 
- Add transaction tracking for AppSignal (>= 2.0, < 4.0)
- Add transaction tracking for Sentry (>= 4.1.0)
- Support simultaneous instrumentation in both APM tools
- Add observability configuration (appsignal_enabled, sentry_enabled)
- Add comprehensive unit tests for APM instrumentation
- Update documentation with usage examples
- Bump version to 0.3.0

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
hieuk09
hieuk09 reviewed Nov 14, 2025
View reviewed changes
Copy link
Collaborator

@hieuk09 hieuk09 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only one nit.

lib/idempotency.rb Outdated
instrumented_block = block

# Wrap with Sentry if enabled
if config.observability.sentry_enabled && defined?(Sentry)
Copy link
Collaborator

@hieuk09 hieuk09 Nov 14, 2025

Choose a reason for hiding this comment

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

Could we do the Sentry check at load time? Like when sentry_enabled is turned on 🤔

Copy link
Collaborator

@hieuk09 hieuk09 Nov 14, 2025

Choose a reason for hiding this comment

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

At that point, we may require the library & check for version to ensure that we don't use outdated version.

@HNKhoi2410
Copy link
Contributor Author

HNKhoi2410 commented Nov 14, 2025

We push the event successfully to Appsignal https://appsignal.com/kaligo/sites/5d9414aa74782013d384396a/performance/incidents/35/samples/5d9414aa74782013d384396a-37299000940492997941763102760

CleanShot 2025-11-14 at 13 51 03@2x

I am verifying if current syntax are correct, then will resolve the comments 🧑‍🤝‍🧑

@HNKhoi2410 HNKhoi2410 force-pushed the feature/add-apm-instrumentation branch from ca101b1 to 078e87f Compare November 18, 2025 08:24
@HNKhoi2410 HNKhoi2410 changed the title Add AppSignal and Sentry APM instrumentation (v0.3.0) Add AppSignal instrumentation (v0.3.0) Nov 19, 2025
@HNKhoi2410 HNKhoi2410 changed the title Add AppSignal instrumentation (v0.3.0) [CA-57] Add AppSignal instrumentation (v0.3.0) Nov 19, 2025
@HNKhoi2410 HNKhoi2410 changed the title [CA-57] Add AppSignal instrumentation (v0.3.0) [CA-39] Add AppSignal instrumentation (v0.3.0) Nov 19, 2025
@HNKhoi2410 HNKhoi2410 force-pushed the feature/add-apm-instrumentation branch from 4d82d63 to ee755e1 Compare November 19, 2025 08:38
vu-hoang-kaligo
vu-hoang-kaligo reviewed Nov 19, 2025
View reviewed changes
Copy link
Collaborator

@vu-hoang-kaligo vu-hoang-kaligo left a comment

Choose a reason for hiding this comment

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

Overall LGTM, only some small nits

vu-hoang-kaligo
vu-hoang-kaligo approved these changes Nov 19, 2025
View reviewed changes
Copy link
Collaborator

@vu-hoang-kaligo vu-hoang-kaligo left a comment

Choose a reason for hiding this comment

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

Can test the implementation after we remove Sentry info 🚀

README.md Outdated
Comment on lines 157 to 167
#### Sentry

The gem can add the `use_cache` method to Sentry performance traces when enabled. This allows you to see the idempotency check as part of your request traces and automatically captures any errors that occur.

To enable Sentry transaction tracking:

```ruby
Idempotency.configure do |config|
config.observability.sentry_enabled = true
end
```
Copy link
Collaborator

@vu-hoang-kaligo vu-hoang-kaligo Nov 19, 2025

Choose a reason for hiding this comment

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

forgot to remove Sentry-related info here :keke:

@HNKhoi2410
Copy link
Contributor Author

HNKhoi2410 commented Nov 20, 2025

Test here https://github.com/Kaligo/nydus_network/pull/8541#issuecomment-3555651250

@HNKhoi2410 HNKhoi2410 merged commit a3093f5 into master Nov 20, 2025
1 check passed
@HNKhoi2410 HNKhoi2410 deleted the feature/add-apm-instrumentation branch November 20, 2025 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hieuk09 hieuk09 hieuk09 left review comments

@vu-hoang-kaligo vu-hoang-kaligo vu-hoang-kaligo approved these changes

Assignees

No one assigned

Labels

code approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HNKhoi2410 @hieuk09 @vu-hoang-kaligo