Skip to content

Bootstrap delegation for wildfly/jboss#231

Merged
realark merged 1 commit into
ark/classloader_feature_branchfrom
ark/fix_wildfly
Feb 15, 2018
Merged

Bootstrap delegation for wildfly/jboss#231
realark merged 1 commit into
ark/classloader_feature_branchfrom
ark/fix_wildfly

Conversation

@realark
Copy link
Copy Markdown
Contributor

@realark realark commented Feb 14, 2018

  • Set system property to tell jboss/wildfly to delegate to datadog bootstrap classes

@realark realark added the type: enhancement Enhancements and improvements label Feb 14, 2018
@realark realark requested a review from tylerbenson February 14, 2018 04:18
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My initial suggestion was to have this in a "transform" block... Similar to what we do for injecting helper classes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I initially did that. Problem is, bytebuddy thinks transformers which return null have failed. This was causing us to log an error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd suggest using a StringBuilder with the existing value the start of the StringBuilder. This seems like a lot of unnecessary string concatenation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For code paths that are only hit once string concatenation isn't a big deal, but using a string builder is fine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe a comment explaining what is significant about this class?

Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Move code to transform and use string builder.

@realark
Copy link
Copy Markdown
Contributor Author

realark commented Feb 14, 2018

My comment was lost in the diff. Reposting.

I initially used a transformer to set the property. Problem is, bytebuddy thinks transformers which return null have failed. This was causing us to log an error.

It's harmless, but I think it would confuse support if they saw a failed transform error in the logs.

Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

Looks good. Out of curiosity, any idea what versions of JBoss this is required for?

@realark
Copy link
Copy Markdown
Contributor Author

realark commented Feb 15, 2018

It seems to have been around for a while. The version I'm testing against goes back a few years (I had to use an old version to stay java 7 compatible).

@realark realark merged commit e3d76d9 into ark/classloader_feature_branch Feb 15, 2018
@realark realark deleted the ark/fix_wildfly branch February 15, 2018 00:54
@tylerbenson tylerbenson added this to the 0.4.0 milestone Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants