Skip to content
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

Need a way to pass Subject for the SNS payload #72

Open
MythosRaconteur opened this issue Jul 20, 2017 · 9 comments
Open

Need a way to pass Subject for the SNS payload #72

MythosRaconteur opened this issue Jul 20, 2017 · 9 comments

Comments

@MythosRaconteur
Copy link

MythosRaconteur commented Jul 20, 2017

Hey Jeremy,

Apologies if this is the wrong place to put this, but I wasn't sure where to make the request.

I am using Propono to publish SNS messages, and then consuming them with Lambdas that integrate with various 3rd party services (like Slack, and many others).

Our topics are notifiable elements (essentially business objects), with one Lambda acting as a message broker, figuring out which integration to hit, via resubmission of a SNS package against a modified topic. The modified topic is the original topic plus the integration (i.e. user-slack).

The "Slack" Lambda is triggered from that message and fires of a message to Slack.

My issue is that I need the original path accessible to the broker Lambda, and while I could parse the ARN, it would be much easier to just use the Subject of the SNS package, which seems to always be nil with Propono-generated messages.

I altered the services/publisher.rb script by passing options to the sns.publish method:

def publish_via_sns_syncronously
      begin
        topic = TopicCreator.find_or_create(topic_id)
      rescue => e
        Propono.config.logger.error "Propono [#{id}]: Failed to create topic #{topic_id}: #{e}"
        raise
      end

      begin
        sns.publish(topic.arn, body.to_json, {'Subject' => topic_id})
      rescue => e
        Propono.config.logger.error "Propono [#{id}]: Failed to send via sns: #{e}"
        raise
      end
end

While this works, it would, obviously, be better if it were actually parameterized so I could pass it in with the call to Propono.publish and not have to mod your code.

Is there any issue you see with this? If not, I can put it together in a pull and send it up.

Cheers,

Chris

@iHiD
Copy link
Owner

iHiD commented Jul 21, 2017

Hey @MythosRaconteur - thanks for posting. Let me have a think about this over the weekend and try and work something out. Thanks.

@MythosRaconteur
Copy link
Author

Hey there, I actually pulled my change out of your code and decided to parse the TopicArn for the value. It works fine, but it would be nice if there was a raw value on the JSON object, however that is affected.

Cheers,

C

@malcyL
Copy link
Contributor

malcyL commented Jul 22, 2017

Hi Guys

What if any "subject" from the payload hash was used as the SNS topic subject?

    Propono.publish(
      'user',
      {
        subject: 'my subject'
        entity: 'user',
        action: 'created',
        id: user.id,
      }
    )

I'm thinking that if we add a parameter to publish and use that as the SNS topic subject, it wouldn't be available when reading the message off the SQS queue. That would seem odd to me if I wasn't using lambdas. But having it in the payload there is nothing lost when using pub/sub via the SQS queues.

Great suggestion @MythosRaconteur :-)

Malc

@iHiD
Copy link
Owner

iHiD commented Jul 22, 2017

@malcyL Yeah - I like that idea. My only concern is if subject already has meaning for someone in the payload. Are there rules around what the SNS topic subject can be? If so, the two might clash causing a breaking change (e.g. if SNS-topic-subject has to be ascii and someone is using unicode in their payload).

We could do both, so you can specify subject on publish. If you don't then it uses the subject from the payload. If you want to stop that later behaviour, you could set subject to nil. We could also make it a config option in general?

Thoughts?

@malcyL
Copy link
Contributor

malcyL commented Jul 23, 2017

@iHiD I wonder if both are overkill. I was trying to make it simpler, but two mechanisms and a flag are making it more complicated.

Maybe the subject parameter on publish is the way to go.

I was trying to think of a way for the subject to still be available when you are consuming the messages via Propono.listen_to_queue, but maybe that's not important? From just an API perspective, ignoring how it's implemented, we will have a subject added to publish which is then never exposed when reading. Unless your using lambdas of course!

But I don't want to over complicate it. An optional parameter on publish is more desirable than two mechanisms in my opinion.

@MythosRaconteur
Copy link
Author

Did mean to open a can of worms. ;)

At the end of the day, it is simple enough to split the TopicArn and pull the topic off the end, but it would be convenient if you could just get it directly from its own key.

Is "Subject" on the SNS payload ever used, and if so, what is it intended to be?

Barring the use of that key, perhaps the answer is to just inject "Topic" at the same level?

@malcyL
Copy link
Contributor

malcyL commented Jul 23, 2017

@MythosRaconteur No can of worms - discussion is always good. 🙂

@iHiD
Copy link
Owner

iHiD commented Sep 29, 2017

Hello. v2 is now released and I'm in Propono mindset. Do I need to do anything here? :)

@MythosRaconteur
Copy link
Author

MythosRaconteur commented Sep 29, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants